Hadoop Map/Reduce
  1. Hadoop Map/Reduce
  2. MAPREDUCE-2249

Better to check the reflexive property of the object while overriding equals method of it

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 0.22.0, 0.23.0
    • Fix Version/s: 0.23.0
    • Component/s: None
    • Labels:
      None
    • Environment:

      NA

    • Hadoop Flags:
      Reviewed

      Description

      It is better to check the reflexive property of the object while overriding equals method of it.

      It improves the performance when a heavy object is compared to itself.

      1. MAPREDUCE-2249-1.patch
        0.8 kB
        Bhallamudi Venkata Siva Kamesh
      2. MAPREDUCE-2249.patch
        1 kB
        Devaraj K
      3. MAPREDUCE-2249-2.patch
        1 kB
        Devaraj K

        Activity

        Hide
        Owen O'Malley added a comment -

        Please don't put proposed solutions into the descriptions. They are mailed out as part of every comment. Make a reasonably brief description and add the proposal as the first comment.

        The rest of your description follows:

        For example pls find the below snippet from Counters.java

        Current Implementation
          @Override
          public synchronized boolean equals(Object obj) {
            boolean isEqual = false;
            if (obj != null && obj instanceof Counters) {
              Counters other = (Counters) obj;
              if (size() == other.size()) {
                isEqual = true;
                for (Map.Entry<String, Group> entry : this.counters.entrySet()) {
                  String key = entry.getKey();
                  Group sourceGroup = entry.getValue();
                  Group targetGroup = other.getGroup(key);
                  if (!sourceGroup.equals(targetGroup)) {
                    isEqual = false;
                    break;
                  }
                }
              }
            }
            return isEqual;
          }
          
        Proposed Implementation
          @Override
          public synchronized boolean equals(Object obj) {
            if(this == obj) { 
              return true;
            }
            boolean isEqual = false;
            if (obj != null && obj instanceof Counters) {
              Counters other = (Counters) obj;
              if (size() == other.size()) {
                isEqual = true;
                for (Map.Entry<String, Group> entry : this.counters.entrySet()) {
                  String key = entry.getKey();
                  Group sourceGroup = entry.getValue();
                  Group targetGroup = other.getGroup(key);
                  if (!sourceGroup.equals(targetGroup)) {
                    isEqual = false;
                    break;
                  }
                }
              }
            }
            return isEqual;
          }
          
        Show
        Owen O'Malley added a comment - Please don't put proposed solutions into the descriptions. They are mailed out as part of every comment. Make a reasonably brief description and add the proposal as the first comment. The rest of your description follows: For example pls find the below snippet from Counters.java Current Implementation @Override public synchronized boolean equals( Object obj) { boolean isEqual = false ; if (obj != null && obj instanceof Counters) { Counters other = (Counters) obj; if (size() == other.size()) { isEqual = true ; for (Map.Entry< String , Group> entry : this .counters.entrySet()) { String key = entry.getKey(); Group sourceGroup = entry.getValue(); Group targetGroup = other.getGroup(key); if (!sourceGroup.equals(targetGroup)) { isEqual = false ; break ; } } } } return isEqual; } Proposed Implementation @Override public synchronized boolean equals( Object obj) { if ( this == obj) { return true ; } boolean isEqual = false ; if (obj != null && obj instanceof Counters) { Counters other = (Counters) obj; if (size() == other.size()) { isEqual = true ; for (Map.Entry< String , Group> entry : this .counters.entrySet()) { String key = entry.getKey(); Group sourceGroup = entry.getValue(); Group targetGroup = other.getGroup(key); if (!sourceGroup.equals(targetGroup)) { isEqual = false ; break ; } } } } return isEqual; }
        Hide
        Bhallamudi Venkata Siva Kamesh added a comment -

        Added reflexive check in the equals method.

        Show
        Bhallamudi Venkata Siva Kamesh added a comment - Added reflexive check in the equals method.
        Hide
        Todd Lipcon added a comment -

        Hi Bhallamudi. Looks like your patch introduces some tab characters. The style for Hadoop is whitespace-only, 2-indent. Could you please reformat the change?

        Show
        Todd Lipcon added a comment - Hi Bhallamudi. Looks like your patch introduces some tab characters. The style for Hadoop is whitespace-only, 2-indent. Could you please reformat the change?
        Hide
        Owen O'Malley added a comment -

        We should also change the check for classes to ensure commutativity. Could you change your patch to insist on equality between the classes too?. It should look like:

        if (this == obj) {
          return true;
        } else if (obj == null || obj.getClass() != getClass()) {
          return false;
        } else {
          ...
        }
        
        Show
        Owen O'Malley added a comment - We should also change the check for classes to ensure commutativity. Could you change your patch to insist on equality between the classes too?. It should look like: if ( this == obj) { return true ; } else if (obj == null || obj.getClass() != getClass()) { return false ; } else { ... }
        Hide
        Devaraj K added a comment -

        Provided patch for trunk as per the above comments.

        Show
        Devaraj K added a comment - Provided patch for trunk as per the above comments.
        Hide
        Todd Lipcon added a comment -

        Looks like there are still tab characters in the new patch.

        Show
        Todd Lipcon added a comment - Looks like there are still tab characters in the new patch.
        Hide
        Devaraj K added a comment -

        Corrected the tab characters issue. Thanks Todd.

        Added reflexive and commutative property checks in the equals() methods and moved the synchronization to block level.

        Show
        Devaraj K added a comment - Corrected the tab characters issue. Thanks Todd. Added reflexive and commutative property checks in the equals() methods and moved the synchronization to block level.
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12472831/MAPREDUCE-2249-2.patch
        against trunk revision 1078708.

        +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 new tests are needed for this patch.
        Also please list what manual steps were performed to verify 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 (version 1.3.9) warnings.

        +1 release audit. The applied patch does not increase the total number of release audit warnings.

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

        -1 contrib tests. The patch failed contrib unit tests.

        +1 system test framework. The patch passed system test framework compile.

        Test results: https://hudson.apache.org/hudson/job/PreCommit-MAPREDUCE-Build/123//testReport/
        Findbugs warnings: https://hudson.apache.org/hudson/job/PreCommit-MAPREDUCE-Build/123//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
        Console output: https://hudson.apache.org/hudson/job/PreCommit-MAPREDUCE-Build/123//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/12472831/MAPREDUCE-2249-2.patch against trunk revision 1078708. +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 new tests are needed for this patch. Also please list what manual steps were performed to verify 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 (version 1.3.9) warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. +1 core tests. The patch passed core unit tests. -1 contrib tests. The patch failed contrib unit tests. +1 system test framework. The patch passed system test framework compile. Test results: https://hudson.apache.org/hudson/job/PreCommit-MAPREDUCE-Build/123//testReport/ Findbugs warnings: https://hudson.apache.org/hudson/job/PreCommit-MAPREDUCE-Build/123//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://hudson.apache.org/hudson/job/PreCommit-MAPREDUCE-Build/123//console This message is automatically generated.
        Hide
        Devaraj K added a comment -

        Contrib tests failures are not because of the patch changes. These are failing since Build #105 (Mar 1, 2011 7:05:52 AM).

        Show
        Devaraj K added a comment - Contrib tests failures are not because of the patch changes. These are failing since Build #105 (Mar 1, 2011 7:05:52 AM).
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12472831/MAPREDUCE-2249-2.patch
        against trunk revision 1094093.

        +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 new tests are needed for this patch.
        Also please list what manual steps were performed to verify this patch.

        -1 javadoc. The javadoc tool appears to have generated 1 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 (version 1.3.9) warnings.

        +1 release audit. The applied patch does not increase the total number of release audit warnings.

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

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

        +1 system test framework. The patch passed system test framework compile.

        Test results: https://hudson.apache.org/hudson/job/PreCommit-MAPREDUCE-Build/172//testReport/
        Findbugs warnings: https://hudson.apache.org/hudson/job/PreCommit-MAPREDUCE-Build/172//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
        Console output: https://hudson.apache.org/hudson/job/PreCommit-MAPREDUCE-Build/172//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/12472831/MAPREDUCE-2249-2.patch against trunk revision 1094093. +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 new tests are needed for this patch. Also please list what manual steps were performed to verify this patch. -1 javadoc. The javadoc tool appears to have generated 1 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 (version 1.3.9) warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. +1 core tests. The patch passed core unit tests. +1 contrib tests. The patch passed contrib unit tests. +1 system test framework. The patch passed system test framework compile. Test results: https://hudson.apache.org/hudson/job/PreCommit-MAPREDUCE-Build/172//testReport/ Findbugs warnings: https://hudson.apache.org/hudson/job/PreCommit-MAPREDUCE-Build/172//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://hudson.apache.org/hudson/job/PreCommit-MAPREDUCE-Build/172//console This message is automatically generated.
        Hide
        Todd Lipcon added a comment -

        +1, will commit shortly

        Show
        Todd Lipcon added a comment - +1, will commit shortly
        Hide
        Hudson added a comment -

        Integrated in Hadoop-Mapreduce-trunk-Commit #735 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk-Commit/735/)
        MAPREDUCE-2249. Check the reflexive property of Counters objects when comparing equality. Contributed by Devaraj K.

        todd : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1144097
        Files :

        • /hadoop/common/trunk/mapreduce/CHANGES.txt
        • /hadoop/common/trunk/mapreduce/src/java/org/apache/hadoop/mapred/Counters.java
        Show
        Hudson added a comment - Integrated in Hadoop-Mapreduce-trunk-Commit #735 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk-Commit/735/ ) MAPREDUCE-2249 . Check the reflexive property of Counters objects when comparing equality. Contributed by Devaraj K. todd : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1144097 Files : /hadoop/common/trunk/mapreduce/CHANGES.txt /hadoop/common/trunk/mapreduce/src/java/org/apache/hadoop/mapred/Counters.java
        Hide
        Hudson added a comment -

        Integrated in Hadoop-Mapreduce-trunk #731 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk/731/)
        MAPREDUCE-2249. Check the reflexive property of Counters objects when comparing equality. Contributed by Devaraj K.

        todd : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1144097
        Files :

        • /hadoop/common/trunk/mapreduce/CHANGES.txt
        • /hadoop/common/trunk/mapreduce/src/java/org/apache/hadoop/mapred/Counters.java
        Show
        Hudson added a comment - Integrated in Hadoop-Mapreduce-trunk #731 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk/731/ ) MAPREDUCE-2249 . Check the reflexive property of Counters objects when comparing equality. Contributed by Devaraj K. todd : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1144097 Files : /hadoop/common/trunk/mapreduce/CHANGES.txt /hadoop/common/trunk/mapreduce/src/java/org/apache/hadoop/mapred/Counters.java

          People

          • Assignee:
            Devaraj K
            Reporter:
            Bhallamudi Venkata Siva Kamesh
          • Votes:
            0 Vote for this issue
            Watchers:
            5 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Time Tracking

              Estimated:
              Original Estimate - 48h
              48h
              Remaining:
              Remaining Estimate - 48h
              48h
              Logged:
              Time Spent - Not Specified
              Not Specified

                Development