Findbugs warnings are printed related to equals implementation of several classes

• Key: HADOOP-4210
• Type: Bug
• Status: Closed
• Resolution: Fixed
• Priority: Major
• Assignee: Suresh Srinivas
• Reporter: Suresh Srinivas
• Fix Version/s: 0.20.0

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().

Comments

[Suresh Srinivas]

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()

[Suresh Srinivas]

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.

[Suresh Srinivas]

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.

[Suresh Srinivas]

Updated patch

[Suresh Srinivas]

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.

[Chris Douglas]

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.

[Tsz Wo (Nicholas), SZE]

> 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.

[Chris Douglas]

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.

[Chris Douglas]

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.

[Hadoop QA]

-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.

[Suresh Srinivas]

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

[Chris Douglas]

I just committed this. Thanks, Suresh

[Hudson]

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.

[Robert Chansler]

Edit release note for publication.