Hadoop Common
  1. Hadoop Common
  2. HADOOP-5895

Log message shows -ve number of bytes to be merged in the final merge pass when there are no intermediate merges and merge factor is > number of segments

    Details

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

      Description

      Log message shows -ve number of bytes to be merged in the final merge pass when there are no intermediate merges and the mergeFactor is > total number of segments to be merged. This issue is because of code changes done in HADOOP-5572.

      1. HADOOP-5895.v1.1.patch
        2 kB
        Ravi Gummadi
      2. HADOOP-5895.v1.patch
        1 kB
        Ravi Gummadi
      3. HADOOP-5895.patch
        0.5 kB
        Ravi Gummadi

        Activity

        Hide
        Ravi Gummadi added a comment - - edited

        In computeBytesInMerges, getPassFactor() can return factor > number of segments and even though n++ is done based on condition includeFinalMerge, that is not good enough for the while loop to be executed.

        Attaching patch with the fix. Please review and provide your comments.

        Show
        Ravi Gummadi added a comment - - edited In computeBytesInMerges, getPassFactor() can return factor > number of segments and even though n++ is done based on condition includeFinalMerge, that is not good enough for the while loop to be executed. Attaching patch with the fix. Please review and provide your comments.
        Hide
        Ravi Gummadi added a comment -

        Issue is when mergeFactor > number of segments (not less than).

        Show
        Ravi Gummadi added a comment - Issue is when mergeFactor > number of segments (not less than).
        Hide
        Jothi Padmanabhan added a comment -

        Using a flag to control the number of iterations in includeFinalMerge would be more readable than modifying the variable n itself, no?

        Show
        Jothi Padmanabhan added a comment - Using a flag to control the number of iterations in includeFinalMerge would be more readable than modifying the variable n itself, no?
        Hide
        Ravi Gummadi added a comment -

        Agreed Jothi.
        Here is the patch with a flag doing the work of considering final merge as part of calculation of estimated input bytes of merges.

        Show
        Ravi Gummadi added a comment - Agreed Jothi. Here is the patch with a flag doing the work of considering final merge as part of calculation of estimated input bytes of merges.
        Hide
        Jothi Padmanabhan added a comment -

        +1

        Show
        Jothi Padmanabhan added a comment - +1
        Hide
        Ravi Gummadi added a comment -

        Attaching a new patch fixing a minor issue for the same log message.

        Show
        Ravi Gummadi added a comment - Attaching a new patch fixing a minor issue for the same log message.
        Hide
        Ravi Gummadi added a comment -

        Unit tests passed on my machine.

        ant test-patch gave:

        [exec] -1 overall.
        [exec]
        [exec] +1 @author. The patch does not contain any @author tags.
        [exec]
        [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]
        [exec] +1 javadoc. The javadoc tool did not generate any warning messages.
        [exec]
        [exec] +1 javac. The applied patch does not increase the total number of javac compiler warnings.
        [exec]
        [exec] +1 findbugs. The patch does not introduce any new Findbugs warnings.
        [exec]
        [exec] +1 Eclipse classpath. The patch retains Eclipse classpath integrity.
        [exec]
        [exec] +1 release audit. The applied patch does not increase the total number of release audit warnings.

        Show
        Ravi Gummadi added a comment - Unit tests passed on my machine. ant test-patch gave: [exec] -1 overall. [exec] [exec] +1 @author. The patch does not contain any @author tags. [exec] [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] [exec] +1 javadoc. The javadoc tool did not generate any warning messages. [exec] [exec] +1 javac. The applied patch does not increase the total number of javac compiler warnings. [exec] [exec] +1 findbugs. The patch does not introduce any new Findbugs warnings. [exec] [exec] +1 Eclipse classpath. The patch retains Eclipse classpath integrity. [exec] [exec] +1 release audit. The applied patch does not increase the total number of release audit warnings.
        Hide
        Jothi Padmanabhan added a comment -

        +1 for the last patch

        Show
        Jothi Padmanabhan added a comment - +1 for the last patch
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12408983/HADOOP-5895.v1.1.patch
        against trunk revision 779066.

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

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

        Test results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-vesta.apache.org/411/testReport/
        Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-vesta.apache.org/411/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
        Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-vesta.apache.org/411/artifact/trunk/build/test/checkstyle-errors.html
        Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-vesta.apache.org/411/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/12408983/HADOOP-5895.v1.1.patch against trunk revision 779066. +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 failed core unit tests. -1 contrib tests. The patch failed contrib unit tests. Test results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-vesta.apache.org/411/testReport/ Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-vesta.apache.org/411/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-vesta.apache.org/411/artifact/trunk/build/test/checkstyle-errors.html Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-vesta.apache.org/411/console This message is automatically generated.
        Hide
        Ravi Gummadi added a comment -

        Unit tests failures shown above are not related to this patch.

        Show
        Ravi Gummadi added a comment - Unit tests failures shown above are not related to this patch.
        Hide
        Devaraj Das added a comment -

        I just committed this. Thanks, Ravi!

        Show
        Devaraj Das added a comment - I just committed this. Thanks, Ravi!
        Hide
        Ravi Gummadi added a comment -

        I dont think we need a new testcase for this as the issue is seen in the log message itself just before final merge in map tasks. So I haven't added new testcase.

        Show
        Ravi Gummadi added a comment - I dont think we need a new testcase for this as the issue is seen in the log message itself just before final merge in map tasks. So I haven't added new testcase.
        Hide
        Ravi Gummadi added a comment -

        Also, tested manually with many jobs like loadgen, sort, wordcount(in addition to unit tests) to check if -ve number can be seen in the log message in Merger.

        Show
        Ravi Gummadi added a comment - Also, tested manually with many jobs like loadgen, sort, wordcount(in addition to unit tests) to check if -ve number can be seen in the log message in Merger.
        Hide
        Robert Chansler added a comment -

        Editorial pass over all release notes prior to publication of 0.21. Bug.

        Show
        Robert Chansler added a comment - Editorial pass over all release notes prior to publication of 0.21. Bug.

          People

          • Assignee:
            Ravi Gummadi
            Reporter:
            Ravi Gummadi
          • Votes:
            0 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development