Hadoop Common
  1. Hadoop Common
  2. HADOOP-4210

Findbugs warnings are printed related to equals implementation of several classes

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 0.20.0
    • Component/s: None
    • Labels:
      None
    • Hadoop Flags:
      Incompatible change, Reviewed
    • Release Note:
      Changed public class org.apache.hadoop.mapreduce.ID to be an abstract class. Removed from class org.apache.hadoop.mapreduce.ID the methods public static ID read(DataInput in) and public static ID forName(String str).

      Description

      During compilation - findbugs generates several warnings that indicates bugs in the implementation of equals method. One of the example of this report is:

      Bug type EQ_GETCLASS_AND_CLASS_CONSTANT (click for details)
      In class org.apache.hadoop.mapred.ID
      In method org.apache.hadoop.mapred.ID.equals(Object)
      At ID.java:[line 66]
      Value doesn't work for subtypes

      This class has an equals method that will be broken if it is inherited by subclasses. It compares a class literal with the class of the argument (e.g., in class Foo it might check if Foo.class == o.getClass()). It is better to check if this.getClass() == o.getClass().

      1. HADOOP-4210.patch
        10 kB
        Suresh Srinivas
      2. HADOOP-4210.patch
        8 kB
        Suresh Srinivas
      3. HADOOP4210.patch
        6 kB
        Suresh Srinivas

        Activity

        Hide
        Suresh Srinivas added a comment -

        Fixing the issue reported by changing the implementation of equals to be consistent at all the places where bugs were reported. The implementation of equals ensures the following checks are consistently done before the internal data comparison of the two objects:

        • Returns true if the object references are equal
        • Returns false if the object passed as parameter is null
        • Returns false if the this.getClass() != paramObject.getClass()
        Show
        Suresh Srinivas added a comment - Fixing the issue reported by changing the implementation of equals to be consistent at all the places where bugs were reported. The implementation of equals ensures the following checks are consistently done before the internal data comparison of the two objects: Returns true if the object references are equal Returns false if the object passed as parameter is null Returns false if the this.getClass() != paramObject.getClass()
        Hide
        Suresh Srinivas added a comment -

        After this change, the number of "Correctness Warnings" printed by find bugs reduced from 18 to 8. There is no automated test case written for this change.

        Show
        Suresh Srinivas added a comment - After this change, the number of "Correctness Warnings" printed by find bugs reduced from 18 to 8. There is no automated test case written for this change.
        Hide
        Suresh Srinivas added a comment -

        Incorporating code review comments from Chris:
        1. Use base class equals method in the sub classes
        2. Class ID should not be instantiated. Hence making it abstract and removing the unused static methods that were instantiating the class ID.

        Show
        Suresh Srinivas added a comment - Incorporating code review comments from Chris: 1. Use base class equals method in the sub classes 2. Class ID should not be instantiated. Hence making it abstract and removing the unused static methods that were instantiating the class ID.
        Hide
        Suresh Srinivas added a comment -

        Updated patch

        Show
        Suresh Srinivas added a comment - Updated patch
        Hide
        Suresh Srinivas added a comment -

        Patch passed all the unit tests. The number of findbugs warning for "Correctness Warnings" reduced from 18 to 8.

        Here is the result of test-patch:

              [exec] -1 overall.  
         
              [exec]     +1 @author.  The patch does not contain any @author tags.
         
              [exec]     -1 tests included.  The patch doesn't appear to include any new or modified tests.
              [exec]                         Please justify why no tests are needed for this patch.
         
              [exec]     +1 javadoc.  The javadoc tool did not generate any warning messages.
         
              [exec]     +1 javac.  The applied patch does not increase the total number of javac compiler warnings.
         
              [exec]     +1 findbugs.  The patch does not introduce any new Findbugs warnings.
        
        Show
        Suresh Srinivas added a comment - Patch passed all the unit tests. The number of findbugs warning for "Correctness Warnings" reduced from 18 to 8. Here is the result of test-patch: [exec] -1 overall. [exec] +1 @author. The patch does not contain any @author tags. [exec] -1 tests included. The patch doesn't appear to include any new or modified tests. [exec] Please justify why no tests are needed for this patch. [exec] +1 javadoc. The javadoc tool did not generate any warning messages. [exec] +1 javac. The applied patch does not increase the total number of javac compiler warnings. [exec] +1 findbugs. The patch does not introduce any new Findbugs warnings.
        Hide
        Chris Douglas added a comment -

        Changed fix version to 0.20 (HADOOP-1230, i.e. the o.a.h.mapreduce package, is no longer in the 0.19 branch. If this qualifies as a bug fix, the patch for 0.19 should be regenerated). Also marking this as an incompatible change, as it removes public methods (that shouldn't be- and in core, are not- used) from ID. Do ID and its subclasses need to be a public classes? All can probably be package-private.

        Show
        Chris Douglas added a comment - Changed fix version to 0.20 ( HADOOP-1230 , i.e. the o.a.h.mapreduce package, is no longer in the 0.19 branch. If this qualifies as a bug fix, the patch for 0.19 should be regenerated). Also marking this as an incompatible change, as it removes public methods (that shouldn't be- and in core, are not- used) from ID. Do ID and its subclasses need to be a public classes? All can probably be package-private.
        Hide
        Tsz Wo Nicholas Sze added a comment -

        > Returns false if the this.getClass() != paramObject.getClass()

        For class comparison, should we use equals(...) instead of ==? Otherwise, I am afraid it won't work in dynamic class loading.

        Show
        Tsz Wo Nicholas Sze added a comment - > Returns false if the this.getClass() != paramObject.getClass() For class comparison, should we use equals(...) instead of ==? Otherwise, I am afraid it won't work in dynamic class loading.
        Hide
        Chris Douglas added a comment -

        For class comparison, should we use equals(...) instead of ==? Otherwise, I am afraid it won't work in dynamic class loading.

        java.lang.Class doesn't override equals, so it uses Object::equals, which does exactly this.

        Show
        Chris Douglas added a comment - For class comparison, should we use equals(...) instead of ==? Otherwise, I am afraid it won't work in dynamic class loading. java.lang.Class doesn't override equals, so it uses Object::equals, which does exactly this.
        Hide
        Chris Douglas added a comment -

        Do ID and its subclasses need to be a public classes? All can probably be package-private

        This is wrong; subpackages need access to methods on these IDs. Assuming all test cases pass, +1.

        Show
        Chris Douglas added a comment - Do ID and its subclasses need to be a public classes? All can probably be package-private This is wrong; subpackages need access to methods on these IDs. Assuming all test cases pass, +1.
        Hide
        Hadoop QA added a comment -

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

        +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 failed core unit tests.

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

        Test results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/3402/testReport/
        Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/3402/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
        Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/3402/artifact/trunk/build/test/checkstyle-errors.html
        Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/3402/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/12390452/HADOOP-4210.patch against trunk revision 700322. +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 failed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/3402/testReport/ Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/3402/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/3402/artifact/trunk/build/test/checkstyle-errors.html Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/3402/console This message is automatically generated.
        Hide
        Suresh Srinivas added a comment -

        HDFS test TestLeaseRecovery2 is failing. It is unrelated to this change (it is failing for other patches too)

        Show
        Suresh Srinivas added a comment - HDFS test TestLeaseRecovery2 is failing. It is unrelated to this change (it is failing for other patches too)
        Hide
        Chris Douglas added a comment -

        I just committed this. Thanks, Suresh

        Show
        Chris Douglas added a comment - I just committed this. Thanks, Suresh
        Hide
        Hudson added a comment -

        Integrated in Hadoop-trunk #620 (See http://hudson.zones.apache.org/hudson/job/Hadoop-trunk/620/)
        . Fix findbugs warnings for equals implementations of mapred ID
        classes. Removed public, static ID::read and ID::forName; made ID an
        abstract class. Contributed by Suresh Srinivas.

        Show
        Hudson added a comment - Integrated in Hadoop-trunk #620 (See http://hudson.zones.apache.org/hudson/job/Hadoop-trunk/620/ ) . Fix findbugs warnings for equals implementations of mapred ID classes. Removed public, static ID::read and ID::forName; made ID an abstract class. Contributed by Suresh Srinivas.
        Hide
        Robert Chansler added a comment -

        Edit release note for publication.

        Show
        Robert Chansler added a comment - Edit release note for publication.

          People

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

            Dates

            • Due:
              Created:
              Updated:
              Resolved:

              Development