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

There are some bugs in implementation of MergeManager

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Critical Critical
    • Resolution: Fixed
    • Affects Version/s: 0.23.1
    • Fix Version/s: 0.23.7, 2.1.0-beta, 2.0.4-alpha
    • Component/s: mrv2
    • Labels:
      None
    1. MAPREDUCE-3685.patch
      11 kB
      Ravi Prakash
    2. MAPREDUCE-3685.branch-0.23.patch
      13 kB
      Ravi Prakash
    3. MAPREDUCE-3685.patch
      8 kB
      Ravi Prakash
    4. MAPREDUCE-3685.branch-0.23.patch
      13 kB
      Ravi Prakash
    5. MAPREDUCE-3685.patch
      8 kB
      Ravi Prakash
    6. MAPREDUCE-3685.branch-0.23.patch
      13 kB
      Ravi Prakash
    7. MAPREDUCE-3685.branch-0.23.patch
      14 kB
      Ravi Prakash
    8. MAPREDUCE-3685.patch
      14 kB
      Ravi Prakash
    9. MAPREDUCE-3685.patch
      14 kB
      Ravi Prakash
    10. MAPREDUCE-3685.branch-0.23.patch
      14 kB
      Ravi Prakash
    11. MAPREDUCE-3685.patch
      14 kB
      Ravi Prakash
    12. MAPREDUCE-3685-branch-0.23.1.patch
      13 kB
      anty.rao
    13. MAPREDUCE-3685-branch-0.23.1.patch
      8 kB
      anty.rao
    14. MAPREDUCE-3685-branch-0.23.1.patch
      8 kB
      anty.rao
    15. MAPREDUCE-3685.patch
      8 kB
      anty.rao

      Issue Links

        Activity

        Hide
        anty.rao added a comment -

        There are some bug in MergeManager.java
        1) in the constructor of OnDiskMerger in MergeManager.java on line 472,
        [code]
        public OnDiskMerger(MergeManager<K, V> manager)

        { super(manager, Integer.MAX_VALUE, exceptionReporter); setName("OnDiskMerger - Thread to merge on-disk map-outputs"); setDaemon(true); }

        [code]
        the second parameter mergeFactor in constructor can't be Integer.MAX_VALUE, it should be io.sort.factor. if set mergeFactor to Integer.MAX_VALUE
        the OnDiskMerger will merge all files feed to it at a time , don't show respect to io.sort.factor parameter.

        2) still in MergeManager.java on line 90, the data structure
        [code]Set<Path> onDiskMapOutputs = new TreeSet<Path>();[code]
        is incorrect, you should sort the on disk files by file length, not the uri of Path.
        So the files feed to OnDiskMerger is not sorted by length, hurt the overall performance.

        3) still in MergeManager.java being from line 745
        [code]
        if (0 != onDiskBytes) {
        final int numInMemSegments = memDiskSegments.size();
        diskSegments.addAll(0, memDiskSegments);
        memDiskSegments.clear();
        // Pass mergePhase only if there is a going to be intermediate
        // merges. See comment where mergePhaseFinished is being set
        Progress thisPhase = (mergePhaseFinished) ? null : mergePhase;
        RawKeyValueIterator diskMerge = Merger.merge(
        job, fs, keyClass, valueClass, diskSegments,
        ioSortFactor, numInMemSegments, tmpDir, comparator,
        reporter, false, spilledRecordsCounter, null, thisPhase);
        diskSegments.clear();
        if (0 == finalSegments.size())

        { return diskMerge; }

        finalSegments.add(new Segment<K,V>(
        new RawKVIteratorReader(diskMerge, onDiskBytes), true));
        }
        [code]
        the above bold code which will merge files down to io.sort.factor, maybe have intermediate merge process .
        What's wrong with that you didn't pass in the codec parameter, the intermediate merge process will not compress
        the written file on disk, leading to huge performance degrade.

        Show
        anty.rao added a comment - There are some bug in MergeManager.java 1) in the constructor of OnDiskMerger in MergeManager.java on line 472, [code] public OnDiskMerger(MergeManager<K, V> manager) { super(manager, Integer.MAX_VALUE, exceptionReporter); setName("OnDiskMerger - Thread to merge on-disk map-outputs"); setDaemon(true); } [code] the second parameter mergeFactor in constructor can't be Integer.MAX_VALUE, it should be io.sort.factor. if set mergeFactor to Integer.MAX_VALUE the OnDiskMerger will merge all files feed to it at a time , don't show respect to io.sort.factor parameter. 2) still in MergeManager.java on line 90, the data structure [code] Set<Path> onDiskMapOutputs = new TreeSet<Path>(); [code] is incorrect, you should sort the on disk files by file length, not the uri of Path. So the files feed to OnDiskMerger is not sorted by length, hurt the overall performance. 3) still in MergeManager.java being from line 745 [code] if (0 != onDiskBytes) { final int numInMemSegments = memDiskSegments.size(); diskSegments.addAll(0, memDiskSegments); memDiskSegments.clear(); // Pass mergePhase only if there is a going to be intermediate // merges. See comment where mergePhaseFinished is being set Progress thisPhase = (mergePhaseFinished) ? null : mergePhase; RawKeyValueIterator diskMerge = Merger.merge( job, fs, keyClass, valueClass, diskSegments, ioSortFactor, numInMemSegments, tmpDir, comparator, reporter, false, spilledRecordsCounter, null, thisPhase); diskSegments.clear(); if (0 == finalSegments.size()) { return diskMerge; } finalSegments.add(new Segment<K,V>( new RawKVIteratorReader(diskMerge, onDiskBytes), true)); } [code] the above bold code which will merge files down to io.sort.factor, maybe have intermediate merge process . What's wrong with that you didn't pass in the codec parameter, the intermediate merge process will not compress the written file on disk, leading to huge performance degrade.
        Hide
        anty.rao added a comment -

        Can someone spare some to review this patch?

        Show
        anty.rao added a comment - Can someone spare some to review this patch?
        Hide
        Ravi Prakash added a comment -

        Marking as Patch Available on behalf of anty . I wasn't able to assign the JIRA.

        I'll defer to the other two watching this JIRA for the review since I have very little knowledge about the merge.

        Show
        Ravi Prakash added a comment - Marking as Patch Available on behalf of anty . I wasn't able to assign the JIRA. I'll defer to the other two watching this JIRA for the review since I have very little knowledge about the merge.
        Hide
        Ravi Prakash added a comment -

        anty,

        Can you please edit the attributes of this JIRA? e.g. Affects version and Target versions? The priority of this jira should be atleast Major IMHO.

        Show
        Ravi Prakash added a comment - anty, Can you please edit the attributes of this JIRA? e.g. Affects version and Target versions? The priority of this jira should be atleast Major IMHO.
        Hide
        Hadoop QA added a comment -

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

        +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 patch appears to cause tar ant target to fail.

        +1 eclipse:eclipse. The patch built with eclipse:eclipse.

        -1 findbugs. The patch appears to cause Findbugs (version 1.3.9) to fail.

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

        -1 core tests. The patch failed the unit tests build

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

        Test results: https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/1861//testReport/
        Console output: https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/1861//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/12514083/MAPREDUCE-3685.patch against trunk revision . +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 patch appears to cause tar ant target to fail. +1 eclipse:eclipse. The patch built with eclipse:eclipse. -1 findbugs. The patch appears to cause Findbugs (version 1.3.9) to fail. +1 release audit. The applied patch does not increase the total number of release audit warnings. -1 core tests. The patch failed the unit tests build +1 contrib tests. The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/1861//testReport/ Console output: https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/1861//console This message is automatically generated.
        Hide
        anty.rao added a comment -

        It's first time for me to submit a patch, though I have already read the http://wiki.apache.org/hadoop/HowToContribute, i'm not very clear, Sorry for the all inconvenience

        I regenerate a path for the branch branch-0.23.1,

        Show
        anty.rao added a comment - It's first time for me to submit a patch, though I have already read the http://wiki.apache.org/hadoop/HowToContribute , i'm not very clear, Sorry for the all inconvenience I regenerate a path for the branch branch-0.23.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/12514943/MAPREDUCE-3685-branch-0.23.1.patch
        against trunk revision .

        +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 eclipse:eclipse. The patch built with eclipse:eclipse.

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

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

        Test results: https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/1884//testReport/
        Console output: https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/1884//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/12514943/MAPREDUCE-3685-branch-0.23.1.patch against trunk revision . +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 eclipse:eclipse. The patch built with eclipse:eclipse. +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 unit tests in . +1 contrib tests. The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/1884//testReport/ Console output: https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/1884//console This message is automatically generated.
        Hide
        Ted Yu added a comment -

        @Anty:
        When you reference code, please use curly braces around the word 'code' instead of square brackets.

        Show
        Ted Yu added a comment - @Anty: When you reference code, please use curly braces around the word 'code' instead of square brackets.
        Hide
        Anthony Urso added a comment -

        I am not Anty Rao, but I can't find him on the list of asignees, so I will assign this back to you.

        Show
        Anthony Urso added a comment - I am not Anty Rao, but I can't find him on the list of asignees, so I will assign this back to you.
        Hide
        anty.rao added a comment -

        fix wrong code format

        Show
        anty.rao added a comment - fix wrong code format
        Hide
        anty.rao added a comment -

        @Zhihong
        Thanks,Sorry for the messed up reference code.
        Next time i will keep that in mind.

        Show
        anty.rao added a comment - @Zhihong Thanks,Sorry for the messed up reference code. Next time i will keep that in mind.
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12515113/MAPREDUCE-3685-branch-0.23.1.patch
        against trunk revision .

        +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 patch. The patch command could not apply the patch.

        Console output: https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/1899//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/12515113/MAPREDUCE-3685-branch-0.23.1.patch against trunk revision . +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 patch. The patch command could not apply the patch. Console output: https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/1899//console This message is automatically generated.
        Hide
        anty.rao added a comment -

        Should i also generate a patch against the trunk?

        Show
        anty.rao added a comment - Should i also generate a patch against the trunk?
        Hide
        Ravi Prakash added a comment -

        Hi Anty,

        Thanks for your contribution.

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

        This seems like a pretty important patch to warrant unit tests I know its cumbersome but we all have to

        Also, please try to get the +1 from Hadoop QA.

        -1 patch. The patch command could not apply the patch.

        Did you use the diff command to generate the patch? Is the patch command able to apply your .patch file?

        In case you weren't aware, dev-support/test-patch.sh <YOUR_PATCH_FILE> will run what HadoopQA runs, so you don't have to get so many -1s on the JIRA

        Show
        Ravi Prakash added a comment - Hi Anty, Thanks for your contribution. -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. This seems like a pretty important patch to warrant unit tests I know its cumbersome but we all have to Also, please try to get the +1 from Hadoop QA. -1 patch. The patch command could not apply the patch. Did you use the diff command to generate the patch? Is the patch command able to apply your .patch file? In case you weren't aware, dev-support/test-patch.sh <YOUR_PATCH_FILE> will run what HadoopQA runs, so you don't have to get so many -1s on the JIRA
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12515113/MAPREDUCE-3685-branch-0.23.1.patch
        against trunk revision .

        +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 patch. The patch command could not apply the patch.

        Console output: https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/1936//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/12515113/MAPREDUCE-3685-branch-0.23.1.patch against trunk revision . +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 patch. The patch command could not apply the patch. Console output: https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/1936//console This message is automatically generated.
        Hide
        Schubert Zhang added a comment -

        @Ravi
        Thank you very much. Is someone can and Anty Rao into the assignee list?

        @Anty
        Seems we should add junit code, and make patch for a right version.

        Show
        Schubert Zhang added a comment - @Ravi Thank you very much. Is someone can and Anty Rao into the assignee list? @Anty Seems we should add junit code, and make patch for a right version.
        Hide
        anty.rao added a comment -

        The bugs i reported are very clear on a glance for anyone who know the nuts and bolts of mapreduce implementation.As for the attached patch is also very simple and straight.

        I have done A LOG OF tests comparing the overall mapreduce performance in hadoop-0.23.0 and hadoop-0.20.,,,the results is dispointing for me, mapreduce performance in hadoop-0.23.0 is worse than that in hadoop-0.20.,,

        after digging through the code of hadoop-0.23.0, i was astonished by so many bugs that i reported.

        i was wondering that do they do any performance evaluation before release these codes? Maybe hadoop-0.23.0 isn't used in production cases, currently no one care about the performance of mapreduce.

        Which version should i generate patch against? directly against the trunk

        Show
        anty.rao added a comment - The bugs i reported are very clear on a glance for anyone who know the nuts and bolts of mapreduce implementation.As for the attached patch is also very simple and straight. I have done A LOG OF tests comparing the overall mapreduce performance in hadoop-0.23.0 and hadoop-0.20. ,,,the results is dispointing for me, mapreduce performance in hadoop-0.23.0 is worse than that in hadoop-0.20. ,, after digging through the code of hadoop-0.23.0, i was astonished by so many bugs that i reported. i was wondering that do they do any performance evaluation before release these codes? Maybe hadoop-0.23.0 isn't used in production cases, currently no one care about the performance of mapreduce. Which version should i generate patch against? directly against the trunk
        Hide
        Arun C Murthy added a comment -

        Anty - sorry, I've been busy with other stuff. I'll take a look at this.

        Meanwhile, can you please add a unit test for the same to ensure we don't break these in future? It could be a simple mockito based test too.

        Show
        Arun C Murthy added a comment - Anty - sorry, I've been busy with other stuff. I'll take a look at this. Meanwhile, can you please add a unit test for the same to ensure we don't break these in future? It could be a simple mockito based test too.
        Hide
        Arun C Murthy added a comment -

        Anty, can you pls add some unit tests for this patch so we can commit this asap? Tx!

        Show
        Arun C Murthy added a comment - Anty, can you pls add some unit tests for this patch so we can commit this asap? Tx!
        Hide
        anty.rao added a comment -

        Hi Arun C Murthy
        I have added some tests to prevent such error from happening again,and regenerate the patch.

        Show
        anty.rao added a comment - Hi Arun C Murthy I have added some tests to prevent such error from happening again,and regenerate the patch.
        Hide
        Arun C Murthy added a comment -

        Thanks Anty.

        I'll review the patch one final time and commit after hudson +1s it.

        Show
        Arun C Murthy added a comment - Thanks Anty. I'll review the patch one final time and commit after hudson +1s it.
        Hide
        Ravi Prakash added a comment -

        Submitting patch on behalf of Anty!

        Show
        Ravi Prakash added a comment - Submitting patch on behalf of Anty!
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12522248/MAPREDUCE-3685-branch-0.23.1.patch
        against trunk revision .

        -1 patch. The patch command could not apply the patch.

        Console output: https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/3193//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/12522248/MAPREDUCE-3685-branch-0.23.1.patch against trunk revision . -1 patch . The patch command could not apply the patch. Console output: https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/3193//console This message is automatically generated.
        Hide
        Ravi Prakash added a comment -

        Reformatted patches to apply to branch-0.23 and trunk cleanly

        Show
        Ravi Prakash added a comment - Reformatted patches to apply to branch-0.23 and trunk cleanly
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12563322/MAPREDUCE-3685.branch-0.23.patch
        against trunk revision .

        -1 patch. The patch command could not apply the patch.

        Console output: https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/3197//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/12563322/MAPREDUCE-3685.branch-0.23.patch against trunk revision . -1 patch . The patch command could not apply the patch. Console output: https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/3197//console This message is automatically generated.
        Hide
        Ravi Prakash added a comment -

        Attaching trunk patch by itself

        Show
        Ravi Prakash added a comment - Attaching trunk patch by itself
        Hide
        Hadoop QA added a comment -

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

        +1 @author. The patch does not contain any @author tags.

        +1 tests included. The patch appears to include 1 new or modified test files.

        -1 javac. The applied patch generated 2016 javac compiler warnings (more than the trunk's current 2014 warnings).

        +1 javadoc. The javadoc tool did not generate any warning messages.

        +1 eclipse:eclipse. The patch built with eclipse:eclipse.

        +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 unit tests in hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-core.

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

        Test results: https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/3198//testReport/
        Javac warnings: https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/3198//artifact/trunk/patchprocess/diffJavacWarnings.txt
        Console output: https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/3198//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/12563339/MAPREDUCE-3685.patch against trunk revision . +1 @author . The patch does not contain any @author tags. +1 tests included . The patch appears to include 1 new or modified test files. -1 javac . The applied patch generated 2016 javac compiler warnings (more than the trunk's current 2014 warnings). +1 javadoc . The javadoc tool did not generate any warning messages. +1 eclipse:eclipse . The patch built with eclipse:eclipse. +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 unit tests in hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-core. +1 contrib tests . The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/3198//testReport/ Javac warnings: https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/3198//artifact/trunk/patchprocess/diffJavacWarnings.txt Console output: https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/3198//console This message is automatically generated.
        Hide
        Ravi Prakash added a comment -

        Suppressing warnings for unchecked casts

        Show
        Ravi Prakash added a comment - Suppressing warnings for unchecked casts
        Hide
        Hadoop QA added a comment -

        +1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12563348/MAPREDUCE-3685.patch
        against trunk revision .

        +1 @author. The patch does not contain any @author tags.

        +1 tests included. The patch appears to include 1 new or modified test files.

        +1 javac. The applied patch does not increase the total number of javac compiler warnings.

        +1 javadoc. The javadoc tool did not generate any warning messages.

        +1 eclipse:eclipse. The patch built with eclipse:eclipse.

        +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 unit tests in hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-core.

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

        Test results: https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/3200//testReport/
        Console output: https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/3200//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/12563348/MAPREDUCE-3685.patch against trunk revision . +1 @author . The patch does not contain any @author tags. +1 tests included . The patch appears to include 1 new or modified test files. +1 javac . The applied patch does not increase the total number of javac compiler warnings. +1 javadoc . The javadoc tool did not generate any warning messages. +1 eclipse:eclipse . The patch built with eclipse:eclipse. +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 unit tests in hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-core. +1 contrib tests . The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/3200//testReport/ Console output: https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/3200//console This message is automatically generated.
        Hide
        Ravi Prakash added a comment -

        Similarly for branch-0.23

        @Arun: Can you please review and commit these patches?

        Show
        Ravi Prakash added a comment - Similarly for branch-0.23 @Arun: Can you please review and commit these patches?
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12563360/MAPREDUCE-3685.branch-0.23.patch
        against trunk revision .

        -1 patch. The patch command could not apply the patch.

        Console output: https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/3201//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/12563360/MAPREDUCE-3685.branch-0.23.patch against trunk revision . -1 patch . The patch command could not apply the patch. Console output: https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/3201//console This message is automatically generated.
        Hide
        Ravi Prakash added a comment -

        The last patch is targeted to branch-0.23 and doesn't apply to trunk cleanly. The trunk patch applies cleanly and is +1 from Hadoop QA

        Show
        Ravi Prakash added a comment - The last patch is targeted to branch-0.23 and doesn't apply to trunk cleanly. The trunk patch applies cleanly and is +1 from Hadoop QA
        Hide
        Arun C Murthy added a comment -

        Ravi Prakash The patch looks good. One question: why pass along uncompressed size for the new MapOutput ctor, shouldn't we be using compressed size so we get the smallest on-disk files first?

        Show
        Arun C Murthy added a comment - Ravi Prakash The patch looks good. One question: why pass along uncompressed size for the new MapOutput ctor, shouldn't we be using compressed size so we get the smallest on-disk files first?
        Hide
        Arun C Murthy added a comment -

        One nit: we should use MergeManager.getDiskMapOutputs in OnDiskMerger.merge too... maybe MergeManager.getDiskMapOutputs should just return Path[] and then we can fix MergeManager.finalMerge to use Path[] rather than List<Path>. Thoughts?

        Show
        Arun C Murthy added a comment - One nit: we should use MergeManager.getDiskMapOutputs in OnDiskMerger.merge too... maybe MergeManager.getDiskMapOutputs should just return Path[] and then we can fix MergeManager.finalMerge to use Path[] rather than List<Path>. Thoughts?
        Hide
        anty.rao added a comment -

        Ravi Prakash The patch looks good. One question: why pass along uncompressed size for the new MapOutput ctor, shouldn't we be using compressed size so we get the smallest on-disk files first?

        I agree on this.

        One nit: we should use MergeManager.getDiskMapOutputs in OnDiskMerger.merge too... maybe MergeManager.getDiskMapOutputs should just return Path[] and then we can fix MergeManager.finalMerge to use Path[] rather than List<Path>. Thoughts?

        MergeManager.finalMerge could be better to use List<Path>, b/c method finalMerge may need change the contents of List<Path>;if you use Path[], you have to create a new Path[] to make the modification

        OnDiskMerge.merge can't use MergeManager.getDiskMapOutputs, b/c OnDiskMerge.merge will make changes to MergeManager#onDiskMapOutputs according to its merge policy(e.g. mergeFactor)

        I know these codes are ugly, but i can't think of a better way to fix it.Maybe we should use List<Path> always, but there are many codes using Path[] already.

        Show
        anty.rao added a comment - Ravi Prakash The patch looks good. One question: why pass along uncompressed size for the new MapOutput ctor, shouldn't we be using compressed size so we get the smallest on-disk files first? I agree on this. One nit: we should use MergeManager.getDiskMapOutputs in OnDiskMerger.merge too... maybe MergeManager.getDiskMapOutputs should just return Path[] and then we can fix MergeManager.finalMerge to use Path[] rather than List<Path>. Thoughts? MergeManager.finalMerge could be better to use List<Path>, b/c method finalMerge may need change the contents of List<Path>;if you use Path[], you have to create a new Path[] to make the modification OnDiskMerge.merge can't use MergeManager.getDiskMapOutputs, b/c OnDiskMerge.merge will make changes to MergeManager#onDiskMapOutputs according to its merge policy(e.g. mergeFactor) I know these codes are ugly, but i can't think of a better way to fix it.Maybe we should use List<Path> always, but there are many codes using Path[] already.
        Hide
        Ravi Prakash added a comment -

        Updated patch for branch-0.23

        Show
        Ravi Prakash added a comment - Updated patch for branch-0.23
        Hide
        Ravi Prakash added a comment -

        Updated patch for trunk

        Show
        Ravi Prakash added a comment - Updated patch for trunk
        Hide
        Hadoop QA added a comment -

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

        +1 @author. The patch does not contain any @author tags.

        +1 tests included. The patch appears to include 1 new or modified test files.

        -1 one of tests included doesn't have a timeout.

        +1 javac. The applied patch does not increase the total number of javac compiler warnings.

        +1 javadoc. The javadoc tool did not generate any warning messages.

        +1 eclipse:eclipse. The patch built with eclipse:eclipse.

        -1 findbugs. The patch appears to introduce 1 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 unit tests in hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-core.

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

        Test results: https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/3379//testReport/
        Findbugs warnings: https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/3379//artifact/trunk/patchprocess/newPatchFindbugsWarningshadoop-mapreduce-client-core.html
        Console output: https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/3379//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/12571850/MAPREDUCE-3685.patch against trunk revision . +1 @author . The patch does not contain any @author tags. +1 tests included . The patch appears to include 1 new or modified test files. -1 one of tests included doesn't have a timeout. +1 javac . The applied patch does not increase the total number of javac compiler warnings. +1 javadoc . The javadoc tool did not generate any warning messages. +1 eclipse:eclipse . The patch built with eclipse:eclipse. -1 findbugs . The patch appears to introduce 1 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 unit tests in hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-core. +1 contrib tests . The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/3379//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/3379//artifact/trunk/patchprocess/newPatchFindbugsWarningshadoop-mapreduce-client-core.html Console output: https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/3379//console This message is automatically generated.
        Hide
        Ravi Prakash added a comment -

        Adding timeout to test

        Show
        Ravi Prakash added a comment - Adding timeout to test
        Hide
        Ravi Prakash added a comment -

        Adding timeout to test and following findbug's advice

        Show
        Ravi Prakash added a comment - Adding timeout to test and following findbug's advice
        Hide
        Hadoop QA added a comment -

        +1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12571888/MAPREDUCE-3685.patch
        against trunk revision .

        +1 @author. The patch does not contain any @author tags.

        +1 tests included. The patch appears to include 1 new or modified test files.

        +1 tests included appear to have a timeout.

        +1 javac. The applied patch does not increase the total number of javac compiler warnings.

        +1 javadoc. The javadoc tool did not generate any warning messages.

        +1 eclipse:eclipse. The patch built with eclipse:eclipse.

        +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 unit tests in hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-core.

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

        Test results: https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/3380//testReport/
        Console output: https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/3380//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/12571888/MAPREDUCE-3685.patch against trunk revision . +1 @author . The patch does not contain any @author tags. +1 tests included . The patch appears to include 1 new or modified test files. +1 tests included appear to have a timeout. +1 javac . The applied patch does not increase the total number of javac compiler warnings. +1 javadoc . The javadoc tool did not generate any warning messages. +1 eclipse:eclipse . The patch built with eclipse:eclipse. +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 unit tests in hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-core. +1 contrib tests . The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/3380//testReport/ Console output: https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/3380//console This message is automatically generated.
        Hide
        Ravi Prakash added a comment -

        Sorry! I had forgot to sort based on compressed size. This patch incorporates that suggestion

        Show
        Ravi Prakash added a comment - Sorry! I had forgot to sort based on compressed size. This patch incorporates that suggestion
        Hide
        Ravi Prakash added a comment -

        The same for trunk

        Show
        Ravi Prakash added a comment - The same for trunk
        Hide
        Hadoop QA added a comment -

        +1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12571968/MAPREDUCE-3685.patch
        against trunk revision .

        +1 @author. The patch does not contain any @author tags.

        +1 tests included. The patch appears to include 1 new or modified test files.

        +1 tests included appear to have a timeout.

        +1 javac. The applied patch does not increase the total number of javac compiler warnings.

        +1 javadoc. The javadoc tool did not generate any warning messages.

        +1 eclipse:eclipse. The patch built with eclipse:eclipse.

        +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 unit tests in hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-core.

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

        Test results: https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/3382//testReport/
        Console output: https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/3382//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/12571968/MAPREDUCE-3685.patch against trunk revision . +1 @author . The patch does not contain any @author tags. +1 tests included . The patch appears to include 1 new or modified test files. +1 tests included appear to have a timeout. +1 javac . The applied patch does not increase the total number of javac compiler warnings. +1 javadoc . The javadoc tool did not generate any warning messages. +1 eclipse:eclipse . The patch built with eclipse:eclipse. +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 unit tests in hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-core. +1 contrib tests . The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/3382//testReport/ Console output: https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/3382//console This message is automatically generated.
        Hide
        Ravi Prakash added a comment -

        This is probably more for just my reference than anything. Here's my understanding from reading the code. This is very approximate and may be inaccurate for some cases

        IntermediateMemoryToMemoryMerger - Can be toggled on / off

        • Merges map outputs from memory to memory
        • When is it triggered? (If at all enabled, which it isn't by default) When the number of in memory map outputs > memToMemMergeOutputsThreshold
          I am guessing this was put in on the premise that it might be faster to sort fewer number of streams even in memory. And also we can sort perhaps while waiting to fetch.

        InMemoryMerger

        • Merges map outputs from memory to disk
        • When is it triggered? When storing more map outputs in memory would cause to go over memory allocated for shuffle.
        Show
        Ravi Prakash added a comment - This is probably more for just my reference than anything. Here's my understanding from reading the code. This is very approximate and may be inaccurate for some cases IntermediateMemoryToMemoryMerger - Can be toggled on / off Merges map outputs from memory to memory When is it triggered? (If at all enabled, which it isn't by default) When the number of in memory map outputs > memToMemMergeOutputsThreshold I am guessing this was put in on the premise that it might be faster to sort fewer number of streams even in memory. And also we can sort perhaps while waiting to fetch. InMemoryMerger Merges map outputs from memory to disk When is it triggered? When storing more map outputs in memory would cause to go over memory allocated for shuffle.
        Hide
        Arun C Murthy added a comment -

        +1, I'll commit presently.

        Show
        Arun C Murthy added a comment - +1, I'll commit presently.
        Hide
        Arun C Murthy added a comment -

        I just committed this to trunk, branch-2 and branch-0.23. Thanks Anty and Ravi!

        Show
        Arun C Murthy added a comment - I just committed this to trunk, branch-2 and branch-0.23. Thanks Anty and Ravi!
        Hide
        Hudson added a comment -

        Integrated in Hadoop-trunk-Commit #3422 (See https://builds.apache.org/job/Hadoop-trunk-Commit/3422/)
        MAPREDUCE-3685. Fix bugs in MergeManager to ensure compression codec is appropriately used and that on-disk segments are correctly sorted on file-size. Contributed by Anty Rao and Ravi Prakash. (Revision 1453365)

        Result = SUCCESS
        acmurthy : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1453365
        Files :

        • /hadoop/common/trunk/hadoop-mapreduce-project/CHANGES.txt
        • /hadoop/common/trunk/hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-core/src/main/java/org/apache/hadoop/mapred/Merger.java
        • /hadoop/common/trunk/hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-core/src/main/java/org/apache/hadoop/mapreduce/task/reduce/MergeManagerImpl.java
        • /hadoop/common/trunk/hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-core/src/main/java/org/apache/hadoop/mapreduce/task/reduce/OnDiskMapOutput.java
        • /hadoop/common/trunk/hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-core/src/test/java/org/apache/hadoop/mapreduce/task/reduce/TestMergeManager.java
        Show
        Hudson added a comment - Integrated in Hadoop-trunk-Commit #3422 (See https://builds.apache.org/job/Hadoop-trunk-Commit/3422/ ) MAPREDUCE-3685 . Fix bugs in MergeManager to ensure compression codec is appropriately used and that on-disk segments are correctly sorted on file-size. Contributed by Anty Rao and Ravi Prakash. (Revision 1453365) Result = SUCCESS acmurthy : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1453365 Files : /hadoop/common/trunk/hadoop-mapreduce-project/CHANGES.txt /hadoop/common/trunk/hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-core/src/main/java/org/apache/hadoop/mapred/Merger.java /hadoop/common/trunk/hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-core/src/main/java/org/apache/hadoop/mapreduce/task/reduce/MergeManagerImpl.java /hadoop/common/trunk/hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-core/src/main/java/org/apache/hadoop/mapreduce/task/reduce/OnDiskMapOutput.java /hadoop/common/trunk/hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-core/src/test/java/org/apache/hadoop/mapreduce/task/reduce/TestMergeManager.java
        Hide
        Mariappan Asokan added a comment -

        Hi Ravi,
        I guess I am too late to comment since your patch has been committed already. In any case, I have the following comments since you asked

        • In closeOnDiskFile() the following lines of code
              if (onDiskMapOutputs.size() >= (2 * ioSortFactor - 1)) {
                onDiskMerger.startMerge(onDiskMapOutputs);
              }
          

          can be changed to

              if (onDiskMapOutputs.size() >= ioSortFactor) {
                onDiskMerger.startMerge(onDiskMapOutputs);
              }
          

          Please confirm.

        • In the class CompressAwarePath there is a nit in compareTo(). The following lines:
                  } else if (this.getCompressedSize() > compPath.getCompressedSize()) {
                    return 1;
          

          can be simplified as:

                  } else {
                    return 1;
          

          The set will be partially ordered without an additional compare and without executing the line

                return super.compareTo(obj);
          
        • Since the patch fixes some performance issues, did you have a chance to run some benchmarks that show improvements? I know this will take some time. I will leave it to you.

        – Asokan

        Show
        Mariappan Asokan added a comment - Hi Ravi, I guess I am too late to comment since your patch has been committed already. In any case, I have the following comments since you asked In closeOnDiskFile() the following lines of code if (onDiskMapOutputs.size() >= (2 * ioSortFactor - 1)) { onDiskMerger.startMerge(onDiskMapOutputs); } can be changed to if (onDiskMapOutputs.size() >= ioSortFactor) { onDiskMerger.startMerge(onDiskMapOutputs); } Please confirm. In the class CompressAwarePath there is a nit in compareTo(). The following lines: } else if ( this .getCompressedSize() > compPath.getCompressedSize()) { return 1; can be simplified as: } else { return 1; The set will be partially ordered without an additional compare and without executing the line return super .compareTo(obj); Since the patch fixes some performance issues, did you have a chance to run some benchmarks that show improvements? I know this will take some time. I will leave it to you. – Asokan
        Hide
        Hudson added a comment -

        Integrated in Hadoop-Yarn-trunk #148 (See https://builds.apache.org/job/Hadoop-Yarn-trunk/148/)
        MAPREDUCE-3685. Fix bugs in MergeManager to ensure compression codec is appropriately used and that on-disk segments are correctly sorted on file-size. Contributed by Anty Rao and Ravi Prakash. (Revision 1453365)

        Result = SUCCESS
        acmurthy : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1453365
        Files :

        • /hadoop/common/trunk/hadoop-mapreduce-project/CHANGES.txt
        • /hadoop/common/trunk/hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-core/src/main/java/org/apache/hadoop/mapred/Merger.java
        • /hadoop/common/trunk/hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-core/src/main/java/org/apache/hadoop/mapreduce/task/reduce/MergeManagerImpl.java
        • /hadoop/common/trunk/hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-core/src/main/java/org/apache/hadoop/mapreduce/task/reduce/OnDiskMapOutput.java
        • /hadoop/common/trunk/hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-core/src/test/java/org/apache/hadoop/mapreduce/task/reduce/TestMergeManager.java
        Show
        Hudson added a comment - Integrated in Hadoop-Yarn-trunk #148 (See https://builds.apache.org/job/Hadoop-Yarn-trunk/148/ ) MAPREDUCE-3685 . Fix bugs in MergeManager to ensure compression codec is appropriately used and that on-disk segments are correctly sorted on file-size. Contributed by Anty Rao and Ravi Prakash. (Revision 1453365) Result = SUCCESS acmurthy : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1453365 Files : /hadoop/common/trunk/hadoop-mapreduce-project/CHANGES.txt /hadoop/common/trunk/hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-core/src/main/java/org/apache/hadoop/mapred/Merger.java /hadoop/common/trunk/hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-core/src/main/java/org/apache/hadoop/mapreduce/task/reduce/MergeManagerImpl.java /hadoop/common/trunk/hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-core/src/main/java/org/apache/hadoop/mapreduce/task/reduce/OnDiskMapOutput.java /hadoop/common/trunk/hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-core/src/test/java/org/apache/hadoop/mapreduce/task/reduce/TestMergeManager.java
        Hide
        Hudson added a comment -

        Integrated in Hadoop-Hdfs-0.23-Build #546 (See https://builds.apache.org/job/Hadoop-Hdfs-0.23-Build/546/)
        MAPREDUCE-3685. Fix bugs in MergeManager to ensure compression codec is appropriately used and that on-disk segments are correctly sorted on file-size. Contributed by Anty Rao and Ravi Prakash. (Revision 1453373)

        Result = UNSTABLE
        acmurthy : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1453373
        Files :

        • /hadoop/common/branches/branch-0.23/hadoop-mapreduce-project/CHANGES.txt
        • /hadoop/common/branches/branch-0.23/hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-core/src/main/java/org/apache/hadoop/mapred/Merger.java
        • /hadoop/common/branches/branch-0.23/hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-core/src/main/java/org/apache/hadoop/mapreduce/task/reduce/MapOutput.java
        • /hadoop/common/branches/branch-0.23/hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-core/src/main/java/org/apache/hadoop/mapreduce/task/reduce/MergeManager.java
        • /hadoop/common/branches/branch-0.23/hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-core/src/test/java/org/apache/hadoop/mapreduce/task/reduce/TestMergeManager.java
        Show
        Hudson added a comment - Integrated in Hadoop-Hdfs-0.23-Build #546 (See https://builds.apache.org/job/Hadoop-Hdfs-0.23-Build/546/ ) MAPREDUCE-3685 . Fix bugs in MergeManager to ensure compression codec is appropriately used and that on-disk segments are correctly sorted on file-size. Contributed by Anty Rao and Ravi Prakash. (Revision 1453373) Result = UNSTABLE acmurthy : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1453373 Files : /hadoop/common/branches/branch-0.23/hadoop-mapreduce-project/CHANGES.txt /hadoop/common/branches/branch-0.23/hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-core/src/main/java/org/apache/hadoop/mapred/Merger.java /hadoop/common/branches/branch-0.23/hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-core/src/main/java/org/apache/hadoop/mapreduce/task/reduce/MapOutput.java /hadoop/common/branches/branch-0.23/hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-core/src/main/java/org/apache/hadoop/mapreduce/task/reduce/MergeManager.java /hadoop/common/branches/branch-0.23/hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-core/src/test/java/org/apache/hadoop/mapreduce/task/reduce/TestMergeManager.java
        Hide
        Hudson added a comment -

        Integrated in Hadoop-Hdfs-trunk #1337 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk/1337/)
        MAPREDUCE-3685. Fix bugs in MergeManager to ensure compression codec is appropriately used and that on-disk segments are correctly sorted on file-size. Contributed by Anty Rao and Ravi Prakash. (Revision 1453365)

        Result = SUCCESS
        acmurthy : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1453365
        Files :

        • /hadoop/common/trunk/hadoop-mapreduce-project/CHANGES.txt
        • /hadoop/common/trunk/hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-core/src/main/java/org/apache/hadoop/mapred/Merger.java
        • /hadoop/common/trunk/hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-core/src/main/java/org/apache/hadoop/mapreduce/task/reduce/MergeManagerImpl.java
        • /hadoop/common/trunk/hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-core/src/main/java/org/apache/hadoop/mapreduce/task/reduce/OnDiskMapOutput.java
        • /hadoop/common/trunk/hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-core/src/test/java/org/apache/hadoop/mapreduce/task/reduce/TestMergeManager.java
        Show
        Hudson added a comment - Integrated in Hadoop-Hdfs-trunk #1337 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk/1337/ ) MAPREDUCE-3685 . Fix bugs in MergeManager to ensure compression codec is appropriately used and that on-disk segments are correctly sorted on file-size. Contributed by Anty Rao and Ravi Prakash. (Revision 1453365) Result = SUCCESS acmurthy : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1453365 Files : /hadoop/common/trunk/hadoop-mapreduce-project/CHANGES.txt /hadoop/common/trunk/hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-core/src/main/java/org/apache/hadoop/mapred/Merger.java /hadoop/common/trunk/hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-core/src/main/java/org/apache/hadoop/mapreduce/task/reduce/MergeManagerImpl.java /hadoop/common/trunk/hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-core/src/main/java/org/apache/hadoop/mapreduce/task/reduce/OnDiskMapOutput.java /hadoop/common/trunk/hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-core/src/test/java/org/apache/hadoop/mapreduce/task/reduce/TestMergeManager.java
        Hide
        Hudson added a comment -

        Integrated in Hadoop-Mapreduce-trunk #1365 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk/1365/)
        MAPREDUCE-3685. Fix bugs in MergeManager to ensure compression codec is appropriately used and that on-disk segments are correctly sorted on file-size. Contributed by Anty Rao and Ravi Prakash. (Revision 1453365)

        Result = SUCCESS
        acmurthy : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1453365
        Files :

        • /hadoop/common/trunk/hadoop-mapreduce-project/CHANGES.txt
        • /hadoop/common/trunk/hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-core/src/main/java/org/apache/hadoop/mapred/Merger.java
        • /hadoop/common/trunk/hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-core/src/main/java/org/apache/hadoop/mapreduce/task/reduce/MergeManagerImpl.java
        • /hadoop/common/trunk/hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-core/src/main/java/org/apache/hadoop/mapreduce/task/reduce/OnDiskMapOutput.java
        • /hadoop/common/trunk/hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-core/src/test/java/org/apache/hadoop/mapreduce/task/reduce/TestMergeManager.java
        Show
        Hudson added a comment - Integrated in Hadoop-Mapreduce-trunk #1365 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk/1365/ ) MAPREDUCE-3685 . Fix bugs in MergeManager to ensure compression codec is appropriately used and that on-disk segments are correctly sorted on file-size. Contributed by Anty Rao and Ravi Prakash. (Revision 1453365) Result = SUCCESS acmurthy : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1453365 Files : /hadoop/common/trunk/hadoop-mapreduce-project/CHANGES.txt /hadoop/common/trunk/hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-core/src/main/java/org/apache/hadoop/mapred/Merger.java /hadoop/common/trunk/hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-core/src/main/java/org/apache/hadoop/mapreduce/task/reduce/MergeManagerImpl.java /hadoop/common/trunk/hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-core/src/main/java/org/apache/hadoop/mapreduce/task/reduce/OnDiskMapOutput.java /hadoop/common/trunk/hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-core/src/test/java/org/apache/hadoop/mapreduce/task/reduce/TestMergeManager.java
        Hide
        Ravi Prakash added a comment -

        Hi Mariappan!

        Thanks a lot for your review and comments. I'm sure this is not the last JIRA to go into MergeManagerImpl. If we spot something definitely lets open a new one.

        • Here's my understanding of how the Merge works. Lets assume all the map outputs are on disk. Lets also say that io.sort.factor is set to X. Since we don't want to do any more merges than are necessary, we try to ensure that in the last final merge, there will be X streams to merge. This means that as the reducer starts fetching map outputs, we wait until there are at least 2X-1 map outputs (We don't know how many map outputs we will really get because some maps may not have produced any output). When the number goes over 2X-1, we can be sure that we need an intermediate merge of X streams. This leaves X-1 in onDiskMapOutputs. The X streams are merged into 1. After this merge, together we now have (X-1) + 1 = X streams. When the number of streams > X and < 2X-1, we let the code go to finalMerge, which in itself eventually calls
          Merger.java:645
          if (numSegments <= factor) {
          ....No extra merge needed
          } else {
          ....Do a merge of (number of map outputs) - X
          }
          

          So from my understanding it seems 2X-1 is the correct number. Please let me know if you still think its not.

        • Hmmm.. I didn't know you could give a TreeSet a partial ordering and still get sorted output. The latest javadocs don't say anything, but I found a StackOverflow saying it used to be the case in JDK1.2. Do you know if that is still true?
        • Unfortunately I hadn't run any performance tests. Hopefully we will get the fix on our clusters soon and if we see incredible improvements in performance, I'll try to remember to report back here. This should probably help on large jobs with a lot of maps and we do have quite a few of those
        Show
        Ravi Prakash added a comment - Hi Mariappan! Thanks a lot for your review and comments. I'm sure this is not the last JIRA to go into MergeManagerImpl. If we spot something definitely lets open a new one. Here's my understanding of how the Merge works. Lets assume all the map outputs are on disk. Lets also say that io.sort.factor is set to X. Since we don't want to do any more merges than are necessary, we try to ensure that in the last final merge, there will be X streams to merge. This means that as the reducer starts fetching map outputs, we wait until there are at least 2X-1 map outputs (We don't know how many map outputs we will really get because some maps may not have produced any output). When the number goes over 2X-1, we can be sure that we need an intermediate merge of X streams. This leaves X-1 in onDiskMapOutputs. The X streams are merged into 1. After this merge, together we now have (X-1) + 1 = X streams. When the number of streams > X and < 2X-1, we let the code go to finalMerge, which in itself eventually calls Merger.java:645 if (numSegments <= factor) { ....No extra merge needed } else { ....Do a merge of (number of map outputs) - X } So from my understanding it seems 2X-1 is the correct number. Please let me know if you still think its not. Hmmm.. I didn't know you could give a TreeSet a partial ordering and still get sorted output. The latest javadocs don't say anything, but I found a StackOverflow saying it used to be the case in JDK1.2. Do you know if that is still true? Unfortunately I hadn't run any performance tests. Hopefully we will get the fix on our clusters soon and if we see incredible improvements in performance, I'll try to remember to report back here. This should probably help on large jobs with a lot of maps and we do have quite a few of those
        Hide
        Mariappan Asokan added a comment -

        Hi Ravi,
        You are absolutely right that we want to make sure that in the final merge the number of streams to merge is less than or equal to ioSortFactor. I stand corrected on that. So I should have stated that the change is:

            if (onDiskMapOutputs.size() > ioSortFactor) {
              onDiskMerger.startMerge(onDiskMapOutputs);
            }
        

        Note that I changed ">=" to ">" from my previous suggestion. The rationale for starting the merge early(instead of waiting until 2*ioSortFactor - 1 disk files are created) is to leverage additional overlapped processing. The last merge will end up with merging less than or equal to ioSortFactor disk files.

        For example, suppose io.sort.factor is set to 100 and there are 198 disk files. With the code in your patch, all 198 files will be merged in the final merge with two sequential merge passes one with 100 and the other with 99. With my suggestion, the first 100 would have been merged overlapped with the fetches and in-memory merges. The last merge will merge only 99 files.

        By partial ordering, I did not mean unsorted order. I meant that duplicates will be retained. Perhaps, I confused you with a mathematical term. Simply put, in a partially ordered set, all elements
        are related by "<=". When you compare two elements in the set, you want to make sure that the smaller element is collated first. If one of them is greater than or equal to the other, it is collated second. The Java TreeSet implementation will keep the elements in sorted order with this
        Comparable implementation. BTW, what you have in the patch will work. What I suggested has less code with no unnecessary comparisons. For brevity, you can even code it as:

        return((this.getCompressedSize() < compPath.getCompressedSize()) ? -1 : 1);
        

        IMHO, any performance enhancement should not result in performance regression for any use case. I just wanted to make sure that your patch does not cause any performance regression in some cases. If you have some setup to run performance tests, please go ahead. Otherwise, just ignore my suggestion.

        – Asokan

        Show
        Mariappan Asokan added a comment - Hi Ravi, You are absolutely right that we want to make sure that in the final merge the number of streams to merge is less than or equal to ioSortFactor. I stand corrected on that. So I should have stated that the change is: if (onDiskMapOutputs.size() > ioSortFactor) { onDiskMerger.startMerge(onDiskMapOutputs); } Note that I changed ">=" to ">" from my previous suggestion. The rationale for starting the merge early(instead of waiting until 2* ioSortFactor - 1 disk files are created) is to leverage additional overlapped processing. The last merge will end up with merging less than or equal to ioSortFactor disk files. For example, suppose io.sort.factor is set to 100 and there are 198 disk files. With the code in your patch, all 198 files will be merged in the final merge with two sequential merge passes one with 100 and the other with 99. With my suggestion, the first 100 would have been merged overlapped with the fetches and in-memory merges. The last merge will merge only 99 files. By partial ordering, I did not mean unsorted order. I meant that duplicates will be retained. Perhaps, I confused you with a mathematical term. Simply put, in a partially ordered set, all elements are related by "<=". When you compare two elements in the set, you want to make sure that the smaller element is collated first. If one of them is greater than or equal to the other, it is collated second. The Java TreeSet implementation will keep the elements in sorted order with this Comparable implementation. BTW, what you have in the patch will work. What I suggested has less code with no unnecessary comparisons. For brevity, you can even code it as: return (( this .getCompressedSize() < compPath.getCompressedSize()) ? -1 : 1); IMHO, any performance enhancement should not result in performance regression for any use case. I just wanted to make sure that your patch does not cause any performance regression in some cases. If you have some setup to run performance tests, please go ahead. Otherwise, just ignore my suggestion. – Asokan
        Hide
        Mariappan Asokan added a comment -

        Hi Ravi,
        I looked at the Merger class a little deeper. I think the optimization(for more parallelism) I suggested is a bit aggressive in some cases. For example, if you end up having only 101 files to merge(instead of 198) Merger will merge just 2 files in the first pass and then merge 100 files for the final merge. Now, if there is a genie that can tell us how many disk files we will create during the course of shuffle/merge we can either opt to wait or kick off the merge as soon as we reach the disk file count greater then io.sort.factor. This is something that can be explored later. For example, if we know that the number of mappers is huge compared to io.sort.factor and we do not have enough memory for large in-memory merges we can opt for the optimization I suggested.

        – Asokan

        Show
        Mariappan Asokan added a comment - Hi Ravi, I looked at the Merger class a little deeper. I think the optimization(for more parallelism) I suggested is a bit aggressive in some cases. For example, if you end up having only 101 files to merge(instead of 198) Merger will merge just 2 files in the first pass and then merge 100 files for the final merge. Now, if there is a genie that can tell us how many disk files we will create during the course of shuffle/merge we can either opt to wait or kick off the merge as soon as we reach the disk file count greater then io.sort.factor. This is something that can be explored later. For example, if we know that the number of mappers is huge compared to io.sort.factor and we do not have enough memory for large in-memory merges we can opt for the optimization I suggested. – Asokan
        Hide
        Konstantin Boudnik added a comment -

        This has been clearly backported to 2.0.4-alpha in r1453367. Marking the ticket as such.

        Show
        Konstantin Boudnik added a comment - This has been clearly backported to 2.0.4-alpha in r1453367. Marking the ticket as such.

          People

          • Assignee:
            anty
            Reporter:
            anty.rao
          • Votes:
            1 Vote for this issue
            Watchers:
            16 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development