Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 0.20.0
    • Component/s: conf, fs, record
    • Labels:
      None
    • Hadoop Flags:
      Incompatible change, Reviewed
    • Release Note:
      Removed from class org.apache.hadoop.fs.RawLocalFileSystem deprecated methods public String getName(), public void lock(Path p, boolean shared) and public void release(Path p).

      Description

      Findbugs generates several errors related to unused return values, thread synchronization and ambiguous method calls

      1. HADOOP-4253.patch
        13 kB
        Suresh Srinivas
      2. HADOOP-4253.patch
        12 kB
        Suresh Srinivas
      3. HADOOP-4253.patch
        11 kB
        Suresh Srinivas

        Activity

        Hide
        Robert Chansler added a comment -

        Edit release note for publication.

        Show
        Robert Chansler added a comment - Edit release note for publication.
        Hide
        Hudson added a comment -
        Show
        Hudson added a comment - Integrated in Hadoop-trunk #626 (See http://hudson.zones.apache.org/hudson/job/Hadoop-trunk/626/ )
        Hide
        Johan Oskarsson added a comment -

        I just committed this. Thanks, Suresh!

        Show
        Johan Oskarsson added a comment - I just committed this. Thanks, Suresh!
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12391226/HADOOP-4253.patch
        against trunk revision 700628.

        +1 @author. The patch does not contain any @author tags.

        -1 tests included. The patch doesn't appear to include any new or modified tests.
        Please justify why no tests are needed for this patch.

        +1 javadoc. The javadoc tool did not generate any warning messages.

        +1 javac. The applied patch does not increase the total number of javac compiler warnings.

        +1 findbugs. The patch does not introduce any new Findbugs warnings.

        +1 Eclipse classpath. The patch retains Eclipse classpath integrity.

        +1 core tests. The patch passed core unit tests.

        +1 contrib tests. The patch passed contrib unit tests.

        Test results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/3412/testReport/
        Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/3412/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
        Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/3412/artifact/trunk/build/test/checkstyle-errors.html
        Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/3412/console

        This message is automatically generated.

        Show
        Hadoop QA added a comment - -1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12391226/HADOOP-4253.patch against trunk revision 700628. +1 @author. The patch does not contain any @author tags. -1 tests included. The patch doesn't appear to include any new or modified tests. Please justify why no tests are needed for this patch. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 findbugs. The patch does not introduce any new Findbugs warnings. +1 Eclipse classpath. The patch retains Eclipse classpath integrity. +1 core tests. The patch passed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/3412/testReport/ Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/3412/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/3412/artifact/trunk/build/test/checkstyle-errors.html Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/3412/console This message is automatically generated.
        Hide
        Suresh Srinivas added a comment -

        Thanks for the comments. I have attached a new patch that incorporates the suggested changes.

        BTW changing to this.getName() still results in findbugs warnings. Method call this.getName() should be attributed only to the super class in this case and cannot refer to the outer class. I have reported this as a findbugs bug to keep track of this.

        Show
        Suresh Srinivas added a comment - Thanks for the comments. I have attached a new patch that incorporates the suggested changes. BTW changing to this.getName() still results in findbugs warnings. Method call this.getName() should be attributed only to the super class in this case and cannot refer to the outer class. I have reported this as a findbugs bug to keep track of this.
        Hide
        Tsz Wo Nicholas Sze added a comment -

        > The TaskTracker threads, I'd use this.getName() rather that super.getName(). I think it is clearer what the intent is.

        this.getName() still generates a findbugs warning.

        Show
        Tsz Wo Nicholas Sze added a comment - > The TaskTracker threads, I'd use this.getName() rather that super.getName(). I think it is clearer what the intent is. this.getName() still generates a findbugs warning.
        Hide
        Owen O'Malley added a comment -

        Let's just change this in 0.20.

        Other comments:

        • In JarBuilder, I think it is better to just remove the in.close() from addFileStream, since the addNamedStream already closes it.
        • The TaskTracker threads, I'd use this.getName() rather that super.getName(). I think it is clearer what the intent is.
        Show
        Owen O'Malley added a comment - Let's just change this in 0.20. Other comments: In JarBuilder, I think it is better to just remove the in.close() from addFileStream, since the addNamedStream already closes it. The TaskTracker threads, I'd use this.getName() rather that super.getName(). I think it is clearer what the intent is.
        Hide
        Tsz Wo Nicholas Sze added a comment -

        +1 Patch looks good.

        Need release note since it removed some (deprecated) public APIs

        Show
        Tsz Wo Nicholas Sze added a comment - +1 Patch looks good. Need release note since it removed some (deprecated) public APIs
        Hide
        Suresh Srinivas added a comment -

        Incorporated the following changes suggest by Chris and Nicholas:

        • Reverted few of the files as the changes were not simple. They will be addressed separately.
        • RawLocalFileSystem deprectated method lock, release and related data are removed.
        • TaskTracker.MapEventFetcherThread prints the super class Thread.getName() instead of enclosing class TaskTracker.getName().
        Show
        Suresh Srinivas added a comment - Incorporated the following changes suggest by Chris and Nicholas: Reverted few of the files as the changes were not simple. They will be addressed separately. RawLocalFileSystem deprectated method lock, release and related data are removed. TaskTracker.MapEventFetcherThread prints the super class Thread.getName() instead of enclosing class TaskTracker.getName().
        Hide
        Tsz Wo Nicholas Sze added a comment -

        The patch span over various components including conf, fs, dfs, mapred, record and streaming. I think it is better to divide this issue into sub-issues.

        Show
        Tsz Wo Nicholas Sze added a comment - The patch span over various components including conf, fs, dfs, mapred, record and streaming. I think it is better to divide this issue into sub-issues.
        Hide
        Suresh Srinivas added a comment -

        Changes to fix the following findbugs warnings:

        • Move Stream creation to try block and ensure the stream is closed in finally block
        • DateFormat is not thread safe. Adding synchronization to avoid threading issues.
        • Added synchronized blocks to fix synchronization issues flagged by findbugs
        • Added retrun value checks in some cases to ensure the error returned by calling a method is handled.
        • In TaskTracker, the inner class MapEventsFetcherThread that extends Thread has ambiguous method getName() called. The method getName() is available in the super class Thread and the enclosing class TaskTracker. To avoid the ambiguity, calling the getName() method of the enclosing class.
        Show
        Suresh Srinivas added a comment - Changes to fix the following findbugs warnings: Move Stream creation to try block and ensure the stream is closed in finally block DateFormat is not thread safe. Adding synchronization to avoid threading issues. Added synchronized blocks to fix synchronization issues flagged by findbugs Added retrun value checks in some cases to ensure the error returned by calling a method is handled. In TaskTracker, the inner class MapEventsFetcherThread that extends Thread has ambiguous method getName() called. The method getName() is available in the super class Thread and the enclosing class TaskTracker. To avoid the ambiguity, calling the getName() method of the enclosing class.

          People

          • Assignee:
            Suresh Srinivas
            Reporter:
            Suresh Srinivas
          • Votes:
            0 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Due:
              Created:
              Updated:
              Resolved:

              Development