Hadoop Common
  1. Hadoop Common
  2. HADOOP-5079

HashFunction inadvertently destroys some randomness

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 0.20.0
    • Component/s: util
    • Labels:
      None
    • Hadoop Flags:
      Reviewed

      Description

      HashFunction.hash restricts initval for the next hash to the [0, maxValue) range of the hash indexes returned. This is suboptimal, particularly for larger nbHash and smaller maxValue. Rather we should first set initval, then restrict the range for the result assignment.

      1. hadoop-core-hash.patch
        0.6 kB
        Jonathan Ellis
      2. hadoop-core-hash-2.patch
        0.6 kB
        Jonathan Ellis

        Activity

        Jonathan Ellis created issue -
        Jonathan Ellis made changes -
        Field Original Value New Value
        Attachment hadoop-core-hash.patch [ 12398264 ]
        Jonathan Ellis made changes -
        Status Open [ 1 ] Patch Available [ 10002 ]
        Hide
        stack added a comment -

        Patch looks good to me. Will commit in a day or so unless objection.

        Show
        stack added a comment - Patch looks good to me. Will commit in a day or so unless objection.
        stack made changes -
        Hadoop Flags [Reviewed]
        Fix Version/s 0.20.0 [ 12313438 ]
        stack made changes -
        Status Patch Available [ 10002 ] In Progress [ 3 ]
        stack made changes -
        Assignee stack [ stack ]
        stack made changes -
        Status In Progress [ 3 ] Patch Available [ 10002 ]
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12398264/hadoop-core-hash.patch
        against trunk revision 737253.

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

        Test results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/3756/testReport/
        Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/3756/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
        Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/3756/artifact/trunk/build/test/checkstyle-errors.html
        Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/3756/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/12398264/hadoop-core-hash.patch against trunk revision 737253. +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 failed contrib unit tests. Test results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/3756/testReport/ Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/3756/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/3756/artifact/trunk/build/test/checkstyle-errors.html Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/3756/console This message is automatically generated.
        Hide
        stack added a comment -

        Committed (One line change that improves a random function – hard to write unit tests for). Thanks for the patch Jonathan.

        Show
        stack added a comment - Committed (One line change that improves a random function – hard to write unit tests for). Thanks for the patch Jonathan.
        stack made changes -
        Resolution Fixed [ 1 ]
        Status Patch Available [ 10002 ] Resolved [ 5 ]
        Hide
        stack added a comment -

        Committed to 0.20 branch too.

        Show
        stack added a comment - Committed to 0.20 branch too.
        Hide
        Raghu Angadi added a comment -

        The change log should be only in place : under 0.20 on trunk as well on 0.20 branch.

        Show
        Raghu Angadi added a comment - The change log should be only in place : under 0.20 on trunk as well on 0.20 branch.
        Hide
        stack added a comment -

        Thanks Raghu. I moved the CHANGES.txt entry.

        Show
        stack added a comment - Thanks Raghu. I moved the CHANGES.txt entry.
        Hide
        Jonathan Ellis added a comment -

        My first patch contained a regression: you have to take the remainder before calling Math.abs, since Math.abs(Integer.MIN_VALUE) == Integer.MIN_VALUE still.

        Show
        Jonathan Ellis added a comment - My first patch contained a regression: you have to take the remainder before calling Math.abs, since Math.abs(Integer.MIN_VALUE) == Integer.MIN_VALUE still.
        Jonathan Ellis made changes -
        Attachment hadoop-core-hash-2.patch [ 12399055 ]
        Jonathan Ellis made changes -
        Status Resolved [ 5 ] Reopened [ 4 ]
        Resolution Fixed [ 1 ]
        Jonathan Ellis made changes -
        Hadoop Flags [Reviewed]
        Status Reopened [ 4 ] Patch Available [ 10002 ]
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12399055/hadoop-core-hash-2.patch
        against trunk revision 739416.

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

        Test results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/3779/testReport/
        Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/3779/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
        Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/3779/artifact/trunk/build/test/checkstyle-errors.html
        Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/3779/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/12399055/hadoop-core-hash-2.patch against trunk revision 739416. +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 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. Test results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/3779/testReport/ Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/3779/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/3779/artifact/trunk/build/test/checkstyle-errors.html Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/3779/console This message is automatically generated.
        Hide
        Chris Douglas added a comment -

        Jonathan: please open a new issue to correct the regression.

        Show
        Chris Douglas added a comment - Jonathan: please open a new issue to correct the regression.
        Chris Douglas made changes -
        Hadoop Flags [Reviewed]
        Resolution Fixed [ 1 ]
        Status Patch Available [ 10002 ] Resolved [ 5 ]
        Chris Douglas made changes -
        Assignee stack [ stack ] Jonathan Ellis [ jbellis ]
        Hide
        stack added a comment -

        I took a look at the patch and confirmed Math.abs(Integer.MIN_VALUE is negative.

        Show
        stack added a comment - I took a look at the patch and confirmed Math.abs(Integer.MIN_VALUE is negative.
        Hide
        Jonathan Ellis added a comment -

        Chris, I've reported the problem and attached a patch; I leave it to you to proceed from there as you see best.

        Show
        Jonathan Ellis added a comment - Chris, I've reported the problem and attached a patch; I leave it to you to proceed from there as you see best.
        Jonathan Ellis made changes -
        Assignee Jonathan Ellis [ jbellis ] Chris Douglas [ chris.douglas ]
        Hide
        Chris Douglas added a comment -

        Jonathan-

        • A resolved issue is assigned to the person who did the work, not the person responsible for the next step. Since you attached the patch ultimately committed, the issue should be assigned to you.
        • Generally, we work 1 patch/issue, so reverting is simple and it's easy to audit work. Reverting your original patch, regenerating a repaired patch (since the new patch assumes the prior one has been committed), and re-committing this issue isn't a good use of anyone's time.

        Please create a new issue describing the regression and attach the patch.

        Show
        Chris Douglas added a comment - Jonathan- A resolved issue is assigned to the person who did the work, not the person responsible for the next step. Since you attached the patch ultimately committed, the issue should be assigned to you. Generally, we work 1 patch/issue, so reverting is simple and it's easy to audit work. Reverting your original patch, regenerating a repaired patch (since the new patch assumes the prior one has been committed), and re-committing this issue isn't a good use of anyone's time. Please create a new issue describing the regression and attach the patch.
        Chris Douglas made changes -
        Assignee Chris Douglas [ chris.douglas ] Jonathan Ellis [ jbellis ]
        Hide
        Hudson added a comment -
        Show
        Hudson added a comment - Integrated in Hadoop-trunk #756 (See http://hudson.zones.apache.org/hudson/job/Hadoop-trunk/756/ )
        Hide
        stack added a comment -

        I moved the patch that was added to the reopened issue to HADOOP-5255.

        Show
        stack added a comment - I moved the patch that was added to the reopened issue to HADOOP-5255 .
        Nigel Daley made changes -
        Status Resolved [ 5 ] Closed [ 6 ]

          People

          • Assignee:
            Jonathan Ellis
            Reporter:
            Jonathan Ellis
          • Votes:
            0 Vote for this issue
            Watchers:
            2 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development