Details

    • Hadoop Flags:
      Reviewed
    • Release Note:
      Allow ReduceTask loading a third party plugin for shuffle (and merge) instead of the default shuffle.
    • Target Version/s:

      Description

      Support generic shuffle service as set of two plugins: ShuffleProvider & ShuffleConsumer.
      This will satisfy the following needs:

      1. Better shuffle and merge performance. For example: we are working on shuffle plugin that performs shuffle over RDMA in fast networks (10gE, 40gE, or Infiniband) instead of using the current HTTP shuffle. Based on the fast RDMA shuffle, the plugin can also utilize a suitable merge approach during the intermediate merges. Hence, getting much better performance.
      2. Satisfy MAPREDUCE-3060 - generic shuffle service for avoiding hidden dependency of NodeManager with a specific version of mapreduce shuffle (currently targeted to 0.24.0).

      References:

      1. Hadoop Acceleration through Network Levitated Merging, by Prof. Weikuan Yu from Auburn University with others, http://pasl.eng.auburn.edu/pubs/sc11-netlev.pdf
      2. I am attaching 2 documents with suggested Top Level Design for both plugins (currently, based on 1.0 branch)
      3. I am providing link for downloading UDA - Mellanox's open source plugin that implements generic shuffle service using RDMA and levitated merge. Note: At this phase, the code is in C++ through JNI and you should consider it as beta only. Still, it can serve anyone that wants to implement or contribute to levitated merge. (Please be advised that levitated merge is mostly suit in very fast networks) - http://www.mellanox.com/content/pages.php?pg=products_dyn&product_family=144&menu_section=69
      1. MAPREDUCE-4049--branch-1.patch
        56 kB
        Avner BenHanoch
      2. MAPREDUCE-4049--branch-1.patch
        54 kB
        Avner BenHanoch
      3. MAPREDUCE-4049--branch-1.patch
        27 kB
        Avner BenHanoch
      4. mapreduce-4049.patch
        25 kB
        Avner BenHanoch
      5. Hadoop Shuffle Plugin Design.rtf
        78 kB
        Avner BenHanoch
      6. HADOOP-1.x.y.patch
        20 kB
        Avner BenHanoch

        Issue Links

          Activity

          Hide
          Milind Bhandarkar added a comment -

          There has been an effort recently (https://github.com/hanborq/hadoop) to backport shuffle from 0.23 to 1.0 branch. Would it be pluggable with your patch ?

          Show
          Milind Bhandarkar added a comment - There has been an effort recently ( https://github.com/hanborq/hadoop ) to backport shuffle from 0.23 to 1.0 branch. Would it be pluggable with your patch ?
          Hide
          Avner BenHanoch added a comment -

          Top Level Design for the Shuffle Consumer/Provider Plugin classes

          Show
          Avner BenHanoch added a comment - Top Level Design for the Shuffle Consumer/Provider Plugin classes
          Hide
          Avner BenHanoch added a comment -
          1. I just attached top level design documents for both plugins, based on current 1.0 branch.
          2. I want to have the described plugin-ability (desired with same interface) for all future versions of Hadoop (as mentioned in the Target Version/s field).
          3. On the first phase, I am focusing on the existing 1.0 branch as I know it. In parallel, I'll try to learn what exists in 0.23 or in HDH. Later, I'll be glad to do any adjustments that may be needed for fulfilling the previous point (as much as possible).
          4. I welcome you and other parties, to let me know if there are branch(es) that particularly interest you, so I'll give it priority.
          Show
          Avner BenHanoch added a comment - I just attached top level design documents for both plugins, based on current 1.0 branch. I want to have the described plugin-ability (desired with same interface) for all future versions of Hadoop (as mentioned in the Target Version/s field). On the first phase, I am focusing on the existing 1.0 branch as I know it. In parallel, I'll try to learn what exists in 0.23 or in HDH. Later, I'll be glad to do any adjustments that may be needed for fulfilling the previous point (as much as possible). I welcome you and other parties, to let me know if there are branch(es) that particularly interest you, so I'll give it priority.
          Hide
          Avner BenHanoch added a comment -

          Attached is fix for supporting shuffle consumer & shuffle provider plugins in Hadoop.

          The fix is intended for review.
          Two comments are already known to me:
          1. I changed several members of ReduceTask to be public for allowing easy access by ShuffleConsumerPlugin(s). I will enhance that in future commits.

          2. ShuffleProviderPlugin has currently very few members. Hence, currently, it doesn't support plugins that support security. I will enhance that in future commits.

          Technical info:
          ============
          1. The fix contains 2 new source files (that were splitted from the BIG ReduceTask.java file) and changes in few existing files (One of them is a unit test file).

          2. The fix is based on Hadoop 1.0.0. After your comments I'll be glad to provide enhanced fix for future 1.x.y versions (as well as for 2.x)

          3. The fix is provided in two formats: a) as 2 diff files (src & test), and b) as source files in src.tgz

          Show
          Avner BenHanoch added a comment - Attached is fix for supporting shuffle consumer & shuffle provider plugins in Hadoop. The fix is intended for review. Two comments are already known to me: 1. I changed several members of ReduceTask to be public for allowing easy access by ShuffleConsumerPlugin(s). I will enhance that in future commits. 2. ShuffleProviderPlugin has currently very few members. Hence, currently, it doesn't support plugins that support security. I will enhance that in future commits. Technical info: ============ 1. The fix contains 2 new source files (that were splitted from the BIG ReduceTask.java file) and changes in few existing files (One of them is a unit test file). 2. The fix is based on Hadoop 1.0.0. After your comments I'll be glad to provide enhanced fix for future 1.x.y versions (as well as for 2.x) 3. The fix is provided in two formats: a) as 2 diff files (src & test), and b) as source files in src.tgz
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12521522/test.diff
          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 patch. The patch command could not apply the patch.

          Console output: https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/2161//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/12521522/test.diff 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 patch. The patch command could not apply the patch. Console output: https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/2161//console This message is automatically generated.
          Hide
          Avner BenHanoch added a comment -

          This is same patch as before - this time it should be in correct format for automation (and not just methodological as before)
          Notice that this time it is intended for hadoop-1.0.2

          Show
          Avner BenHanoch added a comment - This is same patch as before - this time it should be in correct format for automation (and not just methodological as before) Notice that this time it is intended for hadoop-1.0.2
          Hide
          Avner BenHanoch added a comment -

          this was only methodological patch and not a formal one, based on svn paths

          Show
          Avner BenHanoch added a comment - this was only methodological patch and not a formal one, based on svn paths
          Hide
          Avner BenHanoch added a comment -

          HADOOP-1.0.2.patch

          Show
          Avner BenHanoch added a comment - HADOOP-1 .0.2.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/12521533/HADOOP-1.0.2.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 patch. The patch command could not apply the patch.

          Console output: https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/2162//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/12521533/HADOOP-1.0.2.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 patch. The patch command could not apply the patch. Console output: https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/2162//console This message is automatically generated.
          Hide
          Avner BenHanoch added a comment -

          This time the patch is after svn add for new files

          Show
          Avner BenHanoch added a comment - This time the patch is after svn add for new files
          Hide
          Avner BenHanoch added a comment -

          patch for 1.0.2 (includes svn add for 3 new files)

          Show
          Avner BenHanoch added a comment - patch for 1.0.2 (includes svn add for 3 new files)
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12521534/HADOOP-1.0.x.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 patch. The patch command could not apply the patch.

          Console output: https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/2163//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/12521534/HADOOP-1.0.x.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 patch. The patch command could not apply the patch. Console output: https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/2163//console This message is automatically generated.
          Hide
          Avner BenHanoch added a comment -

          same patch with different name and with ASF license

          Show
          Avner BenHanoch added a comment - same patch with different name and with ASF license
          Hide
          Avner BenHanoch added a comment -

          same patch with different meta data

          Show
          Avner BenHanoch added a comment - same patch with different meta data
          Hide
          Aaron T. Myers added a comment -

          Hey Avner, apologies if you already know this, but the automated test-patch isn't going to work for any patch made against branch-1. test-patch always blindly tries to apply the patch to trunk, regardless of what branch it's intended for. The source layout is dramatically different between branch-1 and trunk, so it's unlikely that any patch will apply cleanly.

          You should probably run test-patch manually on your box and post the results here.

          Show
          Aaron T. Myers added a comment - Hey Avner, apologies if you already know this, but the automated test-patch isn't going to work for any patch made against branch-1. test-patch always blindly tries to apply the patch to trunk, regardless of what branch it's intended for. The source layout is dramatically different between branch-1 and trunk, so it's unlikely that any patch will apply cleanly. You should probably run test-patch manually on your box and post the results here.
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12521536/MAPREDUCE-4049-branch-1.0.2.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 patch. The patch command could not apply the patch.

          Console output: https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/2164//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/12521536/MAPREDUCE-4049-branch-1.0.2.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 patch. The patch command could not apply the patch. Console output: https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/2164//console This message is automatically generated.
          Hide
          Avner BenHanoch added a comment -

          Thanks Aaron, I was not aware of that and it seems strange to me (also the error message is not helping). The patch is for 1.0.2, not for the trunk.
          It was created using: svn diff src/mapred/ src/test/.
          It runs smoothly in my env.
          I am able to apply it on clean 1.0.2 tree using patch -p0 < patch-file
          I hope it can be reviewed based on the attached source/patch.
          Otherwise, I'll try test-patch after Passover

          Show
          Avner BenHanoch added a comment - Thanks Aaron, I was not aware of that and it seems strange to me (also the error message is not helping). The patch is for 1.0.2, not for the trunk. It was created using: svn diff src/mapred/ src/test/ . It runs smoothly in my env. I am able to apply it on clean 1.0.2 tree using patch -p0 < patch-file I hope it can be reviewed based on the attached source/patch. Otherwise, I'll try test-patch after Passover
          Hide
          Aaron T. Myers added a comment -

          Yea, it's an unfortunate known deficiency of test-patch: HADOOP-7435

          Show
          Aaron T. Myers added a comment - Yea, it's an unfortunate known deficiency of test-patch: HADOOP-7435
          Hide
          Avner BenHanoch added a comment -

          thanks. Meanwhile, I'll ignore those automatic QA messages.

          Show
          Avner BenHanoch added a comment - thanks. Meanwhile, I'll ignore those automatic QA messages.
          Hide
          Matt Foley added a comment -

          This patch was not committed to Hadoop-1.0.2. Corrected the "Fix Versions" and "Target Versions" fields.

          Show
          Matt Foley added a comment - This patch was not committed to Hadoop-1.0.2. Corrected the "Fix Versions" and "Target Versions" fields.
          Hide
          Avner BenHanoch added a comment -

          please advise If there is anything else I need to do. thanks.

          Show
          Avner BenHanoch added a comment - please advise If there is anything else I need to do. thanks.
          Hide
          Mariappan Asokan added a comment -

          Hi Avner,
          I worked on MAPREDUCE-2454(to make sort pluggable in Hadoop) and posted a patch on top of trunk version 1221902 a while back. The patch was created on top of the trunk since ReduceTask.java was already refactored nicely and I was
          advised to work on the trunk version.

          Please take a look at the patch file mapreduce-2454.patch posted in MAPREDUCE-2454. If you want, I can post a patch on top of the latest trunk.

          The patch decoupled the merge from shuffle by creating ShuffleRunner and ShuffleCallback interfaces. The MergeManager implements the ShuffleCallback and the shuffle itself implements ShuffleRunner interface.

          Since you are making shuffle as pluggable, I notice some overlapping changes. If I can be of any assistance to reduce the conflict between our patches, please let me know. Meanwhile, I will go over the details of your patch and get back. Do you have a patch created on top of trunk?

          Also, I would like to hear opinions from other developers who have shown interest in this Jira.

          Show
          Mariappan Asokan added a comment - Hi Avner, I worked on MAPREDUCE-2454 (to make sort pluggable in Hadoop) and posted a patch on top of trunk version 1221902 a while back. The patch was created on top of the trunk since ReduceTask.java was already refactored nicely and I was advised to work on the trunk version. Please take a look at the patch file mapreduce-2454.patch posted in MAPREDUCE-2454 . If you want, I can post a patch on top of the latest trunk. The patch decoupled the merge from shuffle by creating ShuffleRunner and ShuffleCallback interfaces. The MergeManager implements the ShuffleCallback and the shuffle itself implements ShuffleRunner interface. Since you are making shuffle as pluggable, I notice some overlapping changes. If I can be of any assistance to reduce the conflict between our patches, please let me know. Meanwhile, I will go over the details of your patch and get back. Do you have a patch created on top of trunk? Also, I would like to hear opinions from other developers who have shown interest in this Jira.
          Hide
          Avner BenHanoch added a comment -

          Hi Mariappan,
          thanks for your comment. I will gladly merge my patch to the trunk with your patch. However, I would be happy to recieve additional comments before I'll procede deeper. At any case, next week I'll take a look at your patch + I'll submit patches for 1.0.3 & 1.1.x and hopefully patch for 0.23 too.

          After that I'll work on the trunk (and let you know if I need updated patch from you). Hoepufully, meanwhile, we'll have additional feedback.

          Show
          Avner BenHanoch added a comment - Hi Mariappan, thanks for your comment. I will gladly merge my patch to the trunk with your patch. However, I would be happy to recieve additional comments before I'll procede deeper. At any case, next week I'll take a look at your patch + I'll submit patches for 1.0.3 & 1.1.x and hopefully patch for 0.23 too. After that I'll work on the trunk (and let you know if I need updated patch from you). Hoepufully, meanwhile, we'll have additional feedback.
          Hide
          Arun C Murthy added a comment -

          Avner, apologies for the late response. I finally have some cycles to look into this.

          Is the design doc still up-to-date? Tx

          Agree with Ashok that this will be a good complement to MAPREDUCE-2454.

          Show
          Arun C Murthy added a comment - Avner, apologies for the late response. I finally have some cycles to look into this. Is the design doc still up-to-date? Tx Agree with Ashok that this will be a good complement to MAPREDUCE-2454 .
          Hide
          Avner BenHanoch added a comment -

          Hi Arun, The design doc is quite up to date with the following comments:

          a. I slightly renamed few methods. I am sure you'll recognize them easily. (in one case the rename is preparation for future merge with hadoop 0.23 that already contains method with identical functionality as the method I suggested in ShuffleConsumerPlugin)

          b. protected abstract methods were not described in the design doc. In the code you'll find 2 such methods in ShuffleConsumerPlugin. However, public interface was not changed.

          Show
          Avner BenHanoch added a comment - Hi Arun, The design doc is quite up to date with the following comments: a. I slightly renamed few methods. I am sure you'll recognize them easily. (in one case the rename is preparation for future merge with hadoop 0.23 that already contains method with identical functionality as the method I suggested in ShuffleConsumerPlugin) b. protected abstract methods were not described in the design doc. In the code you'll find 2 such methods in ShuffleConsumerPlugin. However, public interface was not changed.
          Hide
          Avner BenHanoch added a comment -

          attached patch patch for the live 1.0.x branch (http://svn.apache.org/repos/asf/hadoop/common/branches/branch-1.0)

          Show
          Avner BenHanoch added a comment - attached patch patch for the live 1.0.x branch ( http://svn.apache.org/repos/asf/hadoop/common/branches/branch-1.0 )
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12522757/HADOOP-1.0.x.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 patch. The patch command could not apply the patch.

          Console output: https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/2224//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/12522757/HADOOP-1.0.x.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 patch. The patch command could not apply the patch. Console output: https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/2224//console This message is automatically generated.
          Hide
          Mariappan Asokan added a comment -

          Hi Avner,
          I have a minor comment:

          • The ShuffleProviderPlugin and ShuffleConsumerPlugin can probably implement java.io.Closeable. In that case you need not declare abstract void close() method in them.

          I am waiting for your patch on top of the trunk version.

          Thanks.

          Show
          Mariappan Asokan added a comment - Hi Avner, I have a minor comment: The ShuffleProviderPlugin and ShuffleConsumerPlugin can probably implement java.io.Closeable . In that case you need not declare abstract void close() method in them. I am waiting for your patch on top of the trunk version. Thanks.
          Hide
          Avner BenHanoch added a comment -

          Hi Mariappan, thanks for your comment.
          I just checked out trunk and started to learn Hadoop 3.0. I am looking for the the place to plug ShuffleProvider there
          My next patch will be to the trunk. However, I think it will take me few weeks to reach there.

          Show
          Avner BenHanoch added a comment - Hi Mariappan, thanks for your comment. I just checked out trunk and started to learn Hadoop 3.0. I am looking for the the place to plug ShuffleProvider there My next patch will be to the trunk. However, I think it will take me few weeks to reach there.
          Hide
          Mariappan Asokan added a comment -

          Hi Avner,
          Following are my additional comments and thoughts:

          • Does someone need to implement ShuffleProviderPlugin and ShuffleConsumerPlugin as a pair? Or can one of them be implemented and the other be left to the default implementaion?
          • For the trunk version, the shuffle provider will probably not require any changes. The shuffle consumer on the other hand has to be separated into two parts: one that deals with tansferring map output data across the network and the other that consumes the transferred data. If you look at the patch I posted in MAPREDUCE-2454, the above are abstracted as ShuffleRunner and ShuffleCallback interfaces respectively. In the refactored code as part of the patch, MergeManager class implements ShuffleCallback and Shuffle class implements ShuffleRunner. I can probably enhance the ShuffleRunner interface as below with an added initialize() method which basically gets all the arguments that current Shuffle constructor gets:
            ShuffleRunner.java
            public interface ShuffleRunner extends ExceptionReporter {
              public void initialize(TaskAttemptID reduceId, JobConf jobConf,
                                     TaskUmbilicalProtocol umbilical,
                                     Reporter reporter,
                                     Counters.Counter shuffledMapsCounter,
                                     Counters.Counter reduceShuffleBytes,
                                     Counters.Counter failedShuffleCounter,
                                     TaskStatus status,
                                     Progress copyPhase,
                                     Task reduceTask);
              public void run(ShuffleCallback shuffleCallback)
                throws IOException, InterruptedException;
            }
            
          • If you want to implement your own merge(using RDMA to get shuffled data as described in the technical paper attached to this Jira) you can implement the interface ReduceSortPlugin in addition to ShuffleRunner. In the ReduceSortPlugin your merge class can implement ShuffleCallback. Currently, since the merge and shuffle are running in separate threads, the synchronization is done by waitForResource() method in ShuffleCallback. If you are using RDMA to fetch map outputs, your merge will be in full control. It has to coordinate with your implementation of ShuffleRunner to fetch specific mapper output you want in the merge.

          If you have any questions, please let me know.

          Show
          Mariappan Asokan added a comment - Hi Avner, Following are my additional comments and thoughts: Does someone need to implement ShuffleProviderPlugin and ShuffleConsumerPlugin as a pair? Or can one of them be implemented and the other be left to the default implementaion? For the trunk version, the shuffle provider will probably not require any changes. The shuffle consumer on the other hand has to be separated into two parts: one that deals with tansferring map output data across the network and the other that consumes the transferred data. If you look at the patch I posted in MAPREDUCE-2454 , the above are abstracted as ShuffleRunner and ShuffleCallback interfaces respectively. In the refactored code as part of the patch, MergeManager class implements ShuffleCallback and Shuffle class implements ShuffleRunner . I can probably enhance the ShuffleRunner interface as below with an added initialize() method which basically gets all the arguments that current Shuffle constructor gets: ShuffleRunner.java public interface ShuffleRunner extends ExceptionReporter { public void initialize(TaskAttemptID reduceId, JobConf jobConf, TaskUmbilicalProtocol umbilical, Reporter reporter, Counters.Counter shuffledMapsCounter, Counters.Counter reduceShuffleBytes, Counters.Counter failedShuffleCounter, TaskStatus status, Progress copyPhase, Task reduceTask); public void run(ShuffleCallback shuffleCallback) throws IOException, InterruptedException; } If you want to implement your own merge(using RDMA to get shuffled data as described in the technical paper attached to this Jira) you can implement the interface ReduceSortPlugin in addition to ShuffleRunner . In the ReduceSortPlugin your merge class can implement ShuffleCallback . Currently, since the merge and shuffle are running in separate threads, the synchronization is done by waitForResource() method in ShuffleCallback . If you are using RDMA to fetch map outputs, your merge will be in full control. It has to coordinate with your implementation of ShuffleRunner to fetch specific mapper output you want in the merge. If you have any questions, please let me know.
          Hide
          Avner BenHanoch added a comment -

          Hi Mariappan, thanks for your thorough comments.

          It will take me time to provide you full answer (hopefully by the end of next week). Hence, I'll start with something short and complete it later:

          1. In some cases it make sense to use consumer & provider plugins as pair (for example, I want the Shuffle to be over RDMA instead of HTTP; hence, I must provide both sides). However, it is not mandatory. The interface (and configuration) allow any combinations of plugins, including one from 3rd party and one from vanilla (the default).

          2&3. It could be that you are right. At this phase I am not 100% sure about that (please allow me additional time). My merge doesn't work on shuffled segments, but on streamed of segments during streaming (perhaps it is described in the technical paper). Hence, I need to check deeper.

          I hope to give you complete answer by the end of next week.

          Show
          Avner BenHanoch added a comment - Hi Mariappan, thanks for your thorough comments. It will take me time to provide you full answer (hopefully by the end of next week). Hence, I'll start with something short and complete it later: 1. In some cases it make sense to use consumer & provider plugins as pair (for example, I want the Shuffle to be over RDMA instead of HTTP; hence, I must provide both sides). However, it is not mandatory. The interface (and configuration) allow any combinations of plugins, including one from 3rd party and one from vanilla (the default). 2&3. It could be that you are right. At this phase I am not 100% sure about that (please allow me additional time). My merge doesn't work on shuffled segments, but on streamed of segments during streaming (perhaps it is described in the technical paper). Hence, I need to check deeper. I hope to give you complete answer by the end of next week.
          Hide
          Matt Foley added a comment -

          Not committed in time for 1.0.3. Please consider continuing the contribution in 1.1 context. Thank you.

          Show
          Matt Foley added a comment - Not committed in time for 1.0.3. Please consider continuing the contribution in 1.1 context. Thank you.
          Hide
          Avner BenHanoch added a comment -

          I am returning to Mariappan last comment with more details:

          Bottom line, I accept your design for the trunk! In the trunk, I don't need anything for ShuffleProvider. For ShuffleConsumer, after your patch for the trunk is accepted, I can implement ReduceSortPlugin and provide my implementation for Shuffle&Merge.

          Still, there are minor thing that I need to add to your patch (if possible, I prefer that you'll do it):
          I would like to have the following classes as public (all are in package org.apache.hadoop.mapreduce.task.reduce): EventFetcher , ShuffleScheduler, MapOutput , ShuffleClientMetrics. The last class also requires changing its CTOR to be public.

          I will be glad to know if it is possible to include my requests in your patch.
          Also, I will be glad to know when your patch (including above requests) will be integrated into trunk.
          After that, I will be happy to know, if it will be possible to backport this patch into hadoop-2.x & hadoop-1.x (for 1.x I can supply my own original Shuffle patch without Merge Plugin, since anyhow it is different system).

          Thanks,
          Avner

          Show
          Avner BenHanoch added a comment - I am returning to Mariappan last comment with more details: Bottom line, I accept your design for the trunk! In the trunk, I don't need anything for ShuffleProvider. For ShuffleConsumer, after your patch for the trunk is accepted, I can implement ReduceSortPlugin and provide my implementation for Shuffle&Merge. Still, there are minor thing that I need to add to your patch (if possible, I prefer that you'll do it): I would like to have the following classes as public (all are in package org.apache.hadoop.mapreduce.task.reduce): EventFetcher , ShuffleScheduler, MapOutput , ShuffleClientMetrics. The last class also requires changing its CTOR to be public. I will be glad to know if it is possible to include my requests in your patch. Also, I will be glad to know when your patch (including above requests) will be integrated into trunk. After that, I will be happy to know, if it will be possible to backport this patch into hadoop-2.x & hadoop-1.x (for 1.x I can supply my own original Shuffle patch without Merge Plugin, since anyhow it is different system). Thanks, Avner
          Hide
          Mariappan Asokan added a comment -

          Hi Avner,
          Thanks for your comments. I can make the following classes public with a caveat that the InterfaceStability is "Unstable" in the annotation: EventFetcher and ShuffleScheduler. I already made the class MapOutput "public abstract". I did not touch ShuffleClientMetrics. Do you mind making the change as part of your patch? Please let me know.

          I can post the patch files corresponding to the current trunk as well as for hadoop-2 branch. I need some time to test my patches(may be a couple of weeks.) Yes, for hadoop-1.x you can go ahead with your original patch since I am not posting any patch for 1.x.

          If you are working with a committer to push your changes, perhaps I can work with him/her to push my contributions as well. Please let me know.

          Thanks.
          Asokan

          Show
          Mariappan Asokan added a comment - Hi Avner, Thanks for your comments. I can make the following classes public with a caveat that the InterfaceStability is "Unstable" in the annotation: EventFetcher and ShuffleScheduler. I already made the class MapOutput "public abstract". I did not touch ShuffleClientMetrics. Do you mind making the change as part of your patch? Please let me know. I can post the patch files corresponding to the current trunk as well as for hadoop-2 branch. I need some time to test my patches(may be a couple of weeks.) Yes, for hadoop-1.x you can go ahead with your original patch since I am not posting any patch for 1.x. If you are working with a committer to push your changes, perhaps I can work with him/her to push my contributions as well. Please let me know. Thanks. Asokan
          Hide
          Avner BenHanoch added a comment -

          Hi Asokan,

          Perhaps I was not clear enough. Your patch for the trunk is good enough for my needs. I can write my RDMA shuffle plugin based on either your patch or based on my patch. Hence, I am not planning to submit additional patch for the trunk on top of your patch.
          (I will only submit patch for 1.x)

          I welcome the watchers of this issue to commit your patch!

          (BTW, regarding my request to make 4 classes public, I don't see any InterfaceStability annotation on these classes. Anyhow, this request is not mandatory for me. Still, if possible, I will be glad to have these classes public. It will enable my plugin to reuse these classes instead of duplicating parts of them. Also, minor correction: I mistakenly wrote MapOutput instead of MapHost)

          Show
          Avner BenHanoch added a comment - Hi Asokan, Perhaps I was not clear enough. Your patch for the trunk is good enough for my needs. I can write my RDMA shuffle plugin based on either your patch or based on my patch. Hence, I am not planning to submit additional patch for the trunk on top of your patch. (I will only submit patch for 1.x) I welcome the watchers of this issue to commit your patch! (BTW, regarding my request to make 4 classes public, I don't see any InterfaceStability annotation on these classes. Anyhow, this request is not mandatory for me. Still, if possible, I will be glad to have these classes public. It will enable my plugin to reuse these classes instead of duplicating parts of them. Also, minor correction: I mistakenly wrote MapOutput instead of MapHost)
          Hide
          Luke Lu added a comment -

          Can any people (authors or related people of the Auburn paper) here who can confirm the Hadoop version used for the tests in the paper? Is it 0.20.2 or something in the 0.20.20x to 1.0.x series?

          Show
          Luke Lu added a comment - Can any people (authors or related people of the Auburn paper) here who can confirm the Hadoop version used for the tests in the paper? Is it 0.20.2 or something in the 0.20.20x to 1.0.x series?
          Hide
          Milind Bhandarkar added a comment -

          Luke,

          the original Auburn Univ work was done (with Mellanox support) in version 0.20.x. However, Avner ported it to 1.0.x, which is the patch attached here. We have been testing it at Greenplum (with both default shuffle plugin, and Mellanox's Unstructured Data Accelerator (UDA) plugin,) and haven't found any issues so far.

          Show
          Milind Bhandarkar added a comment - Luke, the original Auburn Univ work was done (with Mellanox support) in version 0.20.x. However, Avner ported it to 1.0.x, which is the patch attached here. We have been testing it at Greenplum (with both default shuffle plugin, and Mellanox's Unstructured Data Accelerator (UDA) plugin,) and haven't found any issues so far.
          Hide
          Luke Lu added a comment -

          Milind, I'm trying to rule out the numbers for 0.20.203+. Can you confirm? Are there any tests done to compare vanilla 1.0.x and the default plugin?

          Show
          Luke Lu added a comment - Milind, I'm trying to rule out the numbers for 0.20.203+. Can you confirm? Are there any tests done to compare vanilla 1.0.x and the default plugin?
          Hide
          Milind Bhandarkar added a comment -

          Luke, I do not know of any tests done with 0.20.203 (Avner do you have any numbers?). The tests that compared vanilla 1.0.x & with pluggable shuffle (with default plugin) do not show any measurable difference.

          Show
          Milind Bhandarkar added a comment - Luke, I do not know of any tests done with 0.20.203 (Avner do you have any numbers?). The tests that compared vanilla 1.0.x & with pluggable shuffle (with default plugin) do not show any measurable difference.
          Hide
          alex gemini added a comment -

          If the shuffle is based on network,should the reduce start before map?Did mapred.reduce.slowstart.completed.maps affect performance?

          Show
          alex gemini added a comment - If the shuffle is based on network,should the reduce start before map?Did mapred.reduce.slowstart.completed.maps affect performance?
          Hide
          Avner BenHanoch added a comment -

          Luke, Auburn paper is related to hadoop 0.20.2. In Mellanox, we ported it to 0.20.203 and to various 1.0.x versions. I will very soon port it to 1.1. We never saw any measurable difference between the default plugin and the corresponding vanilla hadoop. Please let me know if this answers your question.

          Alex, in Auburn's merge algorithm, reducers start the merge after all mappers completed. Hence, when using the RDMA plugin (that uses that algorithm), it makes sense to put high value for mapred.reduce.slowstart.completed.maps. However, Auburn has variant of their merge algorithm called "hybrid merge" in which reducers start merge in parallel to mappers. Anyhow, all this is not part of the default plugin.

          Show
          Avner BenHanoch added a comment - Luke, Auburn paper is related to hadoop 0.20.2. In Mellanox, we ported it to 0.20.203 and to various 1.0.x versions. I will very soon port it to 1.1. We never saw any measurable difference between the default plugin and the corresponding vanilla hadoop. Please let me know if this answers your question. Alex, in Auburn's merge algorithm, reducers start the merge after all mappers completed. Hence, when using the RDMA plugin (that uses that algorithm), it makes sense to put high value for mapred.reduce.slowstart.completed.maps. However, Auburn has variant of their merge algorithm called "hybrid merge" in which reducers start merge in parallel to mappers. Anyhow, all this is not part of the default plugin.
          Hide
          Avner BenHanoch added a comment -

          Attached patch for hadoop-1.1.
          This is port of my previous patch for 1.1 branch
          The patch was tested in my environment and show identical performance to current hadoop when using the default vanilla plugin.

          Show
          Avner BenHanoch added a comment - Attached patch for hadoop-1.1. This is port of my previous patch for 1.1 branch The patch was tested in my environment and show identical performance to current hadoop when using the default vanilla plugin.
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12530039/HADOOP-1.1.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 patch. The patch command could not apply the patch.

          Console output: https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/2423//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/12530039/HADOOP-1.1.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 patch. The patch command could not apply the patch. Console output: https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/2423//console This message is automatically generated.
          Hide
          Avner BenHanoch added a comment -

          just to clarify:
          I changed the status from "Patch Available" to "Open", just because the patch is for 1.1 and not for the trunk.
          As told me, patches for other branches falsely trigger failure in "Hadoop QA auto tests". However, the patch is safely available for for 1.1 (and for 1.0).

          Show
          Avner BenHanoch added a comment - just to clarify: I changed the status from "Patch Available" to "Open", just because the patch is for 1.1 and not for the trunk. As told me, patches for other branches falsely trigger failure in "Hadoop QA auto tests" . However, the patch is safely available for for 1.1 (and for 1.0).
          Hide
          Mariappan Asokan added a comment -

          Hi Avner,
          I made the trunk patch for MAPREDUCE-2454 available. As part of the patch I made ShuffleClientMetrics as public as you asked for.

          Thanks.
          – Asokan

          Show
          Mariappan Asokan added a comment - Hi Avner, I made the trunk patch for MAPREDUCE-2454 available. As part of the patch I made ShuffleClientMetrics as public as you asked for. Thanks. – Asokan
          Hide
          Avner BenHanoch added a comment -

          This patch replaces all my previous patches. It is written in order to ease code review, by doing just the minimal changes in existing code. I believe anyone can verify this patch at glance!

          (my old patches included design enhancements by moving plugins' shared code out of ReduceCopier into plugins' base class, and by making ReduceCopier a standalone class instead of being inner class of ReduceTask).

          Show
          Avner BenHanoch added a comment - This patch replaces all my previous patches. It is written in order to ease code review, by doing just the minimal changes in existing code. I believe anyone can verify this patch at glance! (my old patches included design enhancements by moving plugins' shared code out of ReduceCopier into plugins' base class, and by making ReduceCopier a standalone class instead of being inner class of ReduceTask).
          Hide
          Arun C Murthy added a comment -

          Avner, apologies for taking this long.

          The patch looks reasonable, and small which is great.

          The concern I have is that this patch introduces an interface (i.e. ShuffleProvider/Consumer) which isn't present in hadoop-2.x. Should we do both hadoop-2 and hadoop-1 simultaneously? Else, this 'feature' will break as soon as we upgrade to hadoop-2.x.

          Other nits:

          1. We should get TaskTracker.MapOutputServlet to implement ShuffleProvider interface, else it's very easy to break an interface if no one in the core implements it. For e.g. I have no idea about ShuffleProvider.taskDone or ShuffleProvider.jobDone are used.
          2. Minor nits: ShuffleProvider is mis-spelt in a couple of places.
          3. We should add the new configs for provider/consumer in mapred-default.xml

          Again, apologies it took me so long to get to your patch and thanks for being super-patient! I'd like to work with you to get this committed asap!

          Show
          Arun C Murthy added a comment - Avner, apologies for taking this long. The patch looks reasonable, and small which is great. The concern I have is that this patch introduces an interface (i.e. ShuffleProvider/Consumer) which isn't present in hadoop-2.x. Should we do both hadoop-2 and hadoop-1 simultaneously? Else, this 'feature' will break as soon as we upgrade to hadoop-2.x. Other nits: We should get TaskTracker.MapOutputServlet to implement ShuffleProvider interface, else it's very easy to break an interface if no one in the core implements it. For e.g. I have no idea about ShuffleProvider.taskDone or ShuffleProvider.jobDone are used. Minor nits: ShuffleProvider is mis-spelt in a couple of places. We should add the new configs for provider/consumer in mapred-default.xml Again, apologies it took me so long to get to your patch and thanks for being super-patient! I'd like to work with you to get this committed asap!
          Hide
          Avner BenHanoch added a comment -

          Hi Arun,

          Thanks for your comperhensive review. I accept all your comments, and I'll provide new patch soon.

          I'll be happy to submit a corresponding patch for hadoop-2. Is it possible to start with hadoop-1 patch in order to have it on time for 1.1.0? - I'll be eager to do the hadoop-2 patch immediately after it.

          1. We use taskDone/jobDone as optional messages to the plugin. We have experimental variant of the plugin that uses these messages for optimizing cache handling.

          2. ok

          3. I'll include mapred-default.xml in the patch. I'll use default values for the the new config.

          Thank you for working with me on it,
          Avner

          Show
          Avner BenHanoch added a comment - Hi Arun, Thanks for your comperhensive review. I accept all your comments, and I'll provide new patch soon. I'll be happy to submit a corresponding patch for hadoop-2. Is it possible to start with hadoop-1 patch in order to have it on time for 1.1.0? - I'll be eager to do the hadoop-2 patch immediately after it. 1. We use taskDone/jobDone as optional messages to the plugin. We have experimental variant of the plugin that uses these messages for optimizing cache handling. 2. ok 3. I'll include mapred-default.xml in the patch. I'll use default values for the the new config. Thank you for working with me on it, Avner
          Hide
          Arun C Murthy added a comment -

          I'll be happy to submit a corresponding patch for hadoop-2. Is it possible to start with hadoop-1 patch in order to have it on time for 1.1.0? - I'll be eager to do the hadoop-2 patch immediately after it.

          Normally we do both simultaneously. However, given our lack of attention on this patch I'd be more than willing to ignore that policy, assuming there are no other objections. OTOH, I'd really appreciate some due-diligence on trunk too; mainly to ensure we don't break compatibility across the versions. I hope that is reasonable?

          We use taskDone/jobDone as optional messages to the plugin. We have experimental variant of the plugin that uses these messages for optimizing cache handling.

          Can you share more details? I'm just worried about our ability to continue to support these while we have no idea what they are used for. Thanks.

          Show
          Arun C Murthy added a comment - I'll be happy to submit a corresponding patch for hadoop-2. Is it possible to start with hadoop-1 patch in order to have it on time for 1.1.0? - I'll be eager to do the hadoop-2 patch immediately after it. Normally we do both simultaneously. However, given our lack of attention on this patch I'd be more than willing to ignore that policy, assuming there are no other objections. OTOH, I'd really appreciate some due-diligence on trunk too; mainly to ensure we don't break compatibility across the versions. I hope that is reasonable? We use taskDone/jobDone as optional messages to the plugin. We have experimental variant of the plugin that uses these messages for optimizing cache handling. Can you share more details? I'm just worried about our ability to continue to support these while we have no idea what they are used for. Thanks.
          Hide
          Arun C Murthy added a comment -

          Also, we should add at least one unit test with an alternate ShuffleProvider/ShuffleConsumer to ensure that we fail fast if we break them.

          Show
          Arun C Murthy added a comment - Also, we should add at least one unit test with an alternate ShuffleProvider/ShuffleConsumer to ensure that we fail fast if we break them.
          Hide
          Todd Lipcon added a comment -

          However, given our lack of attention on this patch I'd be more than willing to ignore that policy, assuming there are no other objections

          I am -0.5 on this. "We didn't pay attention to you for a while" doesn't seem like a valid excuse to break our rules - especially for something that's a new feature, rather than a critical bugfix. I won't veto, just registering my discontent.

          Show
          Todd Lipcon added a comment - However, given our lack of attention on this patch I'd be more than willing to ignore that policy, assuming there are no other objections I am -0.5 on this. "We didn't pay attention to you for a while" doesn't seem like a valid excuse to break our rules - especially for something that's a new feature, rather than a critical bugfix. I won't veto, just registering my discontent.
          Hide
          Avner BenHanoch added a comment -

          Hi Arun,
          With respect to your comments, please notice the following:

          regarding "MapOutputServlet to implement ShuffleProviderPlugin"
          1. I am doing that; however, please notice that at this moment, MapOutputServlet will not be a plugin. Please let me know if you also want me to make it a plugin.
          2. making ShuffleProviderPlugin an interface (instead of abstract class as I wrote it) will requires moving parts of its code to TaskTracker. (very few things)
          3. I have no problem to drop jobDone/taskDone from the interface. My core plugin works without it. (it only served me as hints for cache optimization in experimental variant of the plugin)

          We did some due-diligence on trunk:
          4. ShuffleConsumerPlugin - the interface will be implemented by the Shuffle class. Main change is 'run' method instead of fetchOutputs & createKVIterator methods. In Shuffle class, I moved CTOR work to an 'init' method.
          5. ShuffleProviderPlugin is not needed in the trunk at all. The plugin will be loaded as an AuxiliaryService. We already tried that.

          unit tests
          6. I'll add unit test(s) for both interfaces
          7. this may delay the patch in few days since I never used JUnit (I mainly program in C/C++). I am glad to learn it now. Hence, I may submit patch without tests this week, and few days after that patch with tests.
          8. I understand that I should mention my new test(s) in the mapred section of smoke-tests file. Right?

          Thank you for working with me on that,
          Avner

          Show
          Avner BenHanoch added a comment - Hi Arun, With respect to your comments, please notice the following: regarding "MapOutputServlet to implement ShuffleProviderPlugin" 1. I am doing that; however, please notice that at this moment, MapOutputServlet will not be a plugin. Please let me know if you also want me to make it a plugin. 2. making ShuffleProviderPlugin an interface (instead of abstract class as I wrote it) will requires moving parts of its code to TaskTracker. (very few things) 3. I have no problem to drop jobDone/taskDone from the interface. My core plugin works without it. (it only served me as hints for cache optimization in experimental variant of the plugin) We did some due-diligence on trunk: 4. ShuffleConsumerPlugin - the interface will be implemented by the Shuffle class. Main change is 'run' method instead of fetchOutputs & createKVIterator methods. In Shuffle class, I moved CTOR work to an 'init' method. 5. ShuffleProviderPlugin is not needed in the trunk at all. The plugin will be loaded as an AuxiliaryService. We already tried that. unit tests 6. I'll add unit test(s) for both interfaces 7. this may delay the patch in few days since I never used JUnit (I mainly program in C/C++). I am glad to learn it now. Hence, I may submit patch without tests this week, and few days after that patch with tests. 8. I understand that I should mention my new test(s) in the mapred section of smoke-tests file. Right? Thank you for working with me on that, Avner
          Hide
          Avner BenHanoch added a comment -

          Hi Arun,

          • The attached patch, addresses your comments about: TaskTracker.MapOutputServlet to implement ShuffleProvider interface, spelling, and mapred-default.xml.
          • Next week, I'll submit patch that will add tests.
          • Immediately after that, I'll work on patch for the trunk, based on the due-diligence I did.

          Cheers,
          Avner

          Show
          Avner BenHanoch added a comment - Hi Arun, The attached patch, addresses your comments about: TaskTracker.MapOutputServlet to implement ShuffleProvider interface, spelling, and mapred-default.xml. Next week, I'll submit patch that will add tests. Immediately after that, I'll work on patch for the trunk, based on the due-diligence I did. Cheers, Avner
          Hide
          Avner BenHanoch added a comment -

          Hi Arun,

          The attached patch addresses all your hadoop-1.1 comments, including tests.

          Best regards,
          Avner

          Show
          Avner BenHanoch added a comment - Hi Arun, The attached patch addresses all your hadoop-1.1 comments, including tests. Best regards, Avner
          Hide
          Avner BenHanoch added a comment -

          Finally, a patch for hadoop-trunk!
          It is a short patch that adds ShuffleConsumerPlugin support to hadoop core.
          A ShuffleConsumerPlugin works either with the builtin ShuffleHandler or with 3rd party ShuffleProviders which are loaded as AuxiliaryServices.

          On a separate note, please consider the following:

          • Due to hard-coded expressions in hadoop code, it currently can’t benefit from 3rd party ShuffleProviders .
            The current TaskAttemptImpl.java code explicitly call: serviceData.put (ShuffleHandler.MAPREDUCE_SHUFFLE_SERVICEID, ...) and ignores any additional AuxiliaryService. Hence, 3rd party AuxillaryServices can’t receive APPLICATION_INIT events.
          • I consider the above limitation is separate to the patch purpose, hence, I used a workaround in my environment and excluded it from my patch.
            Please let me know if you would like me to handle this limitation. I can approach it either through this JIRA issue, or in a separate one. I believe that this limitation should not delay this patch commit.
          Show
          Avner BenHanoch added a comment - Finally, a patch for hadoop-trunk! It is a short patch that adds ShuffleConsumerPlugin support to hadoop core. A ShuffleConsumerPlugin works either with the builtin ShuffleHandler or with 3rd party ShuffleProviders which are loaded as AuxiliaryServices. On a separate note, please consider the following: Due to hard-coded expressions in hadoop code, it currently can’t benefit from 3rd party ShuffleProviders . The current TaskAttemptImpl.java code explicitly call: serviceData.put (ShuffleHandler.MAPREDUCE_SHUFFLE_SERVICEID, ...) and ignores any additional AuxiliaryService. Hence, 3rd party AuxillaryServices can’t receive APPLICATION_INIT events. I consider the above limitation is separate to the patch purpose, hence, I used a workaround in my environment and excluded it from my patch. Please let me know if you would like me to handle this limitation. I can approach it either through this JIRA issue, or in a separate one. I believe that this limitation should not delay this patch commit.
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12542769/mapreduce-4049.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 2059 javac compiler warnings (more than the trunk's current 2058 warnings).

          -1 javadoc. The javadoc tool appears to have generated 1 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/2782//testReport/
          Javac warnings: https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/2782//artifact/trunk/patchprocess/diffJavacWarnings.txt
          Console output: https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/2782//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/12542769/mapreduce-4049.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 2059 javac compiler warnings (more than the trunk's current 2058 warnings). -1 javadoc. The javadoc tool appears to have generated 1 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/2782//testReport/ Javac warnings: https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/2782//artifact/trunk/patchprocess/diffJavacWarnings.txt Console output: https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/2782//console This message is automatically generated.
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12542778/mapreduce-4049.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 2059 javac compiler warnings (more than the trunk's current 2058 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/2783//testReport/
          Javac warnings: https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/2783//artifact/trunk/patchprocess/diffJavacWarnings.txt
          Console output: https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/2783//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/12542778/mapreduce-4049.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 2059 javac compiler warnings (more than the trunk's current 2058 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/2783//testReport/ Javac warnings: https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/2783//artifact/trunk/patchprocess/diffJavacWarnings.txt Console output: https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/2783//console This message is automatically generated.
          Hide
          chackaravarthy added a comment -

          Hi Avner,

          A ShuffleConsumerPlugin works either with the builtin ShuffleHandler or with 3rd party ShuffleProviders which are loaded as AuxiliaryServices.

          Guess you have wrongly mentioned as ShuffleConsumerPlugin instead of ShuffleProviderPlugin
          correct me if I am wrong

          Thanks

          Show
          chackaravarthy added a comment - Hi Avner, A ShuffleConsumerPlugin works either with the builtin ShuffleHandler or with 3rd party ShuffleProviders which are loaded as AuxiliaryServices. Guess you have wrongly mentioned as ShuffleConsumerPlugin instead of ShuffleProviderPlugin correct me if I am wrong Thanks
          Hide
          Avner BenHanoch added a comment -

          Hi chackaravarthy,

          I am correcting you: A ShuffleConsumerPlugin can ask (and merge) MOF files from either the builtin ShuffleHandler or from 3rd party ShuffleProviders which are loaded as AuxiliaryServices.

          Thanks for your comment. Please let me know if I need to clarify anything else.

          Show
          Avner BenHanoch added a comment - Hi chackaravarthy, I am correcting you: A ShuffleConsumerPlugin can ask (and merge) MOF files from either the builtin ShuffleHandler or from 3rd party ShuffleProviders which are loaded as AuxiliaryServices. Thanks for your comment. Please let me know if I need to clarify anything else.
          Hide
          chackaravarthy added a comment -

          Avner,

          yeah I got it now. thanks for clarifying.
          And I have got one more doubt.

          Here we are making ShuffleProvider and ShuffleConsumer as pluggable
          So, getting map output has been made as pluggable.
          Is it possible to make writing map output also pluggable?

          Thanks

          Show
          chackaravarthy added a comment - Avner, yeah I got it now. thanks for clarifying. And I have got one more doubt. Here we are making ShuffleProvider and ShuffleConsumer as pluggable So, getting map output has been made as pluggable. Is it possible to make writing map output also pluggable? Thanks
          Hide
          Avner BenHanoch added a comment -

          Hi chackaravarthy,

          I think your suggestion is a good idea. However, this JIRA issue is devoted to shuffle. Hence, I recommend that you promote your idea in a separate JIRA issue.
          In case you start it, please notify me. I would like to watch and perhaps contribute to it.

          Avner

          Show
          Avner BenHanoch added a comment - Hi chackaravarthy, I think your suggestion is a good idea. However, this JIRA issue is devoted to shuffle. Hence, I recommend that you promote your idea in a separate JIRA issue. In case you start it, please notify me. I would like to watch and perhaps contribute to it. Avner
          Hide
          chackaravarthy added a comment -

          Avner,

          yeah sure, I will let you know when I start working on that. Thanks for your interest.

          And regarding

          Due to hard-coded expressions in hadoop code, it currently can’t benefit from 3rd party ShuffleProviders .
          The current TaskAttemptImpl.java code explicitly call: serviceData.put (ShuffleHandler.MAPREDUCE_SHUFFLE_SERVICEID, ...) and ignores any additional AuxiliaryService. Hence, 3rd party AuxillaryServices can’t receive APPLICATION_INIT events
          I consider the above limitation is separate to the patch purpose, hence, I used a workaround in my environment and excluded it from my patch.
          Please let me know if you would like me to handle this limitation. I can approach it either through this JIRA issue, or in a separate one. I believe that this limitation should not delay this patch commit.

          I wish that you can handle this limitation also through this JIRA itself so that both shuffle consuming and providing will be pluggable and other provider implementation also can work. its just my wish

          And one doubt,

          So, won't be there any new interface ShuffleProviderPlugin in mrv2 as it is already pluggable by adding as part of auxiliary services?

          Thanks

          Show
          chackaravarthy added a comment - Avner, yeah sure, I will let you know when I start working on that. Thanks for your interest. And regarding Due to hard-coded expressions in hadoop code, it currently can’t benefit from 3rd party ShuffleProviders . The current TaskAttemptImpl.java code explicitly call: serviceData.put (ShuffleHandler.MAPREDUCE_SHUFFLE_SERVICEID, ...) and ignores any additional AuxiliaryService. Hence, 3rd party AuxillaryServices can’t receive APPLICATION_INIT events I consider the above limitation is separate to the patch purpose, hence, I used a workaround in my environment and excluded it from my patch. Please let me know if you would like me to handle this limitation. I can approach it either through this JIRA issue, or in a separate one. I believe that this limitation should not delay this patch commit. I wish that you can handle this limitation also through this JIRA itself so that both shuffle consuming and providing will be pluggable and other provider implementation also can work. its just my wish And one doubt, So, won't be there any new interface ShuffleProviderPlugin in mrv2 as it is already pluggable by adding as part of auxiliary services? Thanks
          Hide
          Avner BenHanoch added a comment -

          regarding,

          I wish that you can handle this limitation also through this JIRA itself so that both shuffle consuming and providing will be pluggable and other provider implementation also can work. its just my wish

          I am intending to handle it anyway. I also need it. I just said that the consumer side is standalone and worth commit as is.
          In order to commit the fix for the TaskAttemptImpl.java limitation, I just need to know the desired behavior. I see two possible ways:

          • I can call serviceData.put(...) for any auxiliary service
          • We can create new entry in XML, so we'll have 2 sets of AuxiliaryServices, one that gets APPLICATION_INIT events and one that does not get them.
            If someone tells me the desired way, I will submit it very soon!

          regarding,

          So, won't be there any new interface ShuffleProviderPlugin in mrv2 as it is already pluggable by adding as part of auxiliary services?

          right. It is not required in mrv2. Yarn arch already provides this functionality thru auxiliary services.

          On a separate note,
          Can someone please tell me how to see javac warnings of Unit Tests? My patch is currently rejected because of such a warning. I want to make sure this warning is cleaned before I resubmit the patch. I am using "mvn test ..." for compiling and running tests. However, this hides javac output. Do you know of a way to see it.
          Thanks!

          Show
          Avner BenHanoch added a comment - regarding, I wish that you can handle this limitation also through this JIRA itself so that both shuffle consuming and providing will be pluggable and other provider implementation also can work. its just my wish I am intending to handle it anyway. I also need it. I just said that the consumer side is standalone and worth commit as is. In order to commit the fix for the TaskAttemptImpl.java limitation, I just need to know the desired behavior. I see two possible ways: I can call serviceData.put(...) for any auxiliary service We can create new entry in XML, so we'll have 2 sets of AuxiliaryServices, one that gets APPLICATION_INIT events and one that does not get them. If someone tells me the desired way, I will submit it very soon! regarding, So, won't be there any new interface ShuffleProviderPlugin in mrv2 as it is already pluggable by adding as part of auxiliary services? right. It is not required in mrv2. Yarn arch already provides this functionality thru auxiliary services. On a separate note, Can someone please tell me how to see javac warnings of Unit Tests? My patch is currently rejected because of such a warning. I want to make sure this warning is cleaned before I resubmit the patch. I am using "mvn test ..." for compiling and running tests. However, this hides javac output. Do you know of a way to see it. Thanks!
          Hide
          Avner BenHanoch added a comment -

          A patch for hadoop-trunk!
          It is a short patch that adds ShuffleConsumerPlugin support to hadoop core.
          A ShuffleConsumerPlugin shuffles (and merges) MOFs from either the builtin ShuffleHandler or from 3rd party ShuffleProviders which can be loaded as AuxiliaryServices.

          On a separate note, please consider the following:

          • Due to hard-coded expression in hadoop code, it currently can’t benefit from 3rd party ShuffleProviders .
            The current TaskAttemptImpl.java code explicitly call: serviceData.put (ShuffleHandler.MAPREDUCE_SHUFFLE_SERVICEID, ...) and ignores any additional AuxiliaryService. Hence, 3rd party AuxillaryServices can’t receive APPLICATION_INIT events.
          • I used a workaround for this limitation in my environment and excluded it from my patch.
            Please let me know if you would like me to handle this limitation. I can approach it either through this JIRA issue, or in a separate one. I believe that this limitation should not delay this patch commit, since ShuffleConsumer stands for itself.
          Show
          Avner BenHanoch added a comment - A patch for hadoop-trunk! It is a short patch that adds ShuffleConsumerPlugin support to hadoop core. A ShuffleConsumerPlugin shuffles (and merges) MOFs from either the builtin ShuffleHandler or from 3rd party ShuffleProviders which can be loaded as AuxiliaryServices. On a separate note, please consider the following: Due to hard-coded expression in hadoop code, it currently can’t benefit from 3rd party ShuffleProviders . The current TaskAttemptImpl.java code explicitly call: serviceData.put (ShuffleHandler.MAPREDUCE_SHUFFLE_SERVICEID, ...) and ignores any additional AuxiliaryService. Hence, 3rd party AuxillaryServices can’t receive APPLICATION_INIT events. I used a workaround for this limitation in my environment and excluded it from my patch. Please let me know if you would like me to handle this limitation. I can approach it either through this JIRA issue, or in a separate one. I believe that this limitation should not delay this patch commit, since ShuffleConsumer stands for 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/12542917/mapreduce-4049.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/2788//testReport/
          Console output: https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/2788//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/12542917/mapreduce-4049.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/2788//testReport/ Console output: https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/2788//console This message is automatically generated.
          Hide
          Mariappan Asokan added a comment -

          Hi Avner,
          Please take a look at MAPREDUCE-2454 patch. There, I wanted to decouple the merge from shuffle to make the reduce sort pluggable. However, presumably you want to retain that coupling for a reason namely RDMA merge which takes care of both shuffle and merge. We seem to have conflict of interest here

          In order to resolve this conflict, I have the following suggestion:

          We can make the ShuffleRunner which is defined as below as pluggable:

          ShuffleRunner.java
          public interface ShuffleRunner extends ExceptionReporter {
            public void run(ShuffleCallback shuffleCallback)
              throws IOException, InterruptedException;
          }
          

          where ShuffleCallback is defined as:

          ShuffleCallback.java
          public interface ShuffleCallback {
            public void waitForResource() throws InterruptedException;
            public MapOutput reserve(TaskAttemptID mapId, long shuffleSize, int fetcherId)
              throws IOException;
            public void close() throws Throwable;
          }
          

          With MAPREDUCE-2454 patch, MergeManager or external sort implementations implement ShuffleCallback.

          When createShuffleRunner() in ReduceTask is called(see the MAPREDUCE-2454 patch), it can return either the default implementation which is an instance of Shuffle class itself or a plugin implementation.

          Now, in order to do the RDMA merge, you can implement ShuffleRunner, ShuffleCallback, and SortPlugin. Your ShuffleRunner and ShuffleCallback implementations will do nothing. SortPlugin will use DefaultMapSortPlugin for map side sort and you provide your ReduceSortPlugin implementation which is going to do the RDMA merge.

          How does this sound? I would like to hear from Arun and others on this idea.

          Thanks.

          – Asokan

          Show
          Mariappan Asokan added a comment - Hi Avner, Please take a look at MAPREDUCE-2454 patch. There, I wanted to decouple the merge from shuffle to make the reduce sort pluggable. However, presumably you want to retain that coupling for a reason namely RDMA merge which takes care of both shuffle and merge. We seem to have conflict of interest here In order to resolve this conflict, I have the following suggestion: We can make the ShuffleRunner which is defined as below as pluggable: ShuffleRunner.java public interface ShuffleRunner extends ExceptionReporter { public void run(ShuffleCallback shuffleCallback) throws IOException, InterruptedException; } where ShuffleCallback is defined as: ShuffleCallback.java public interface ShuffleCallback { public void waitForResource() throws InterruptedException; public MapOutput reserve(TaskAttemptID mapId, long shuffleSize, int fetcherId) throws IOException; public void close() throws Throwable; } With MAPREDUCE-2454 patch, MergeManager or external sort implementations implement ShuffleCallback . When createShuffleRunner() in ReduceTask is called(see the MAPREDUCE-2454 patch), it can return either the default implementation which is an instance of Shuffle class itself or a plugin implementation. Now, in order to do the RDMA merge, you can implement ShuffleRunner , ShuffleCallback , and SortPlugin . Your ShuffleRunner and ShuffleCallback implementations will do nothing. SortPlugin will use DefaultMapSortPlugin for map side sort and you provide your ReduceSortPlugin implementation which is going to do the RDMA merge. How does this sound? I would like to hear from Arun and others on this idea. Thanks. – Asokan
          Hide
          Avner BenHanoch added a comment -

          Hi Asokan,
          I don’t have conflict of interests with you. 4 months ago, I already welcomed the watchers of this issue to help you commit your patch.
          RDMA is not coupled with any merge and there is no such a thing "RDMA merge". It is current Hadoop that couples shuffle with merge. You can be relaxed. I don’t “want to retain that coupling”. My opinion is that your decoupling is correct idea and I encourage it.
          My patch passed code review for hadoop-1 and left with a request to do both hadoop-2 and hadoop-1 simultaneously. Few days ago, I submitted the patch to the trunk and already passed “Automatic QA”. I am currently waiting for code review for trunk version.

          Asokan,
          Your patch contains more than 7,000 rows, while my patch is only 400 rows. I don’t want to wait till your patch passes Automatic QA, and code review, and additional rounds.
          I have no problem with the design you suggested me. However, this design can't work with the current trunk architecture, since in your design, shuffle.run() returns void and not iterator (you rely on merger to return the iterator).
          I suggest that you’ll continue with your patch on top of my patch. In case, you’ll need my help with the integration, I will be honored to assist.
          I am open to any idea that you or someone else may have.

          Thanks,
          Avner

          Show
          Avner BenHanoch added a comment - Hi Asokan, I don’t have conflict of interests with you. 4 months ago, I already welcomed the watchers of this issue to help you commit your patch. RDMA is not coupled with any merge and there is no such a thing "RDMA merge". It is current Hadoop that couples shuffle with merge. You can be relaxed. I don’t “want to retain that coupling”. My opinion is that your decoupling is correct idea and I encourage it. My patch passed code review for hadoop-1 and left with a request to do both hadoop-2 and hadoop-1 simultaneously. Few days ago, I submitted the patch to the trunk and already passed “Automatic QA”. I am currently waiting for code review for trunk version. Asokan, Your patch contains more than 7,000 rows, while my patch is only 400 rows. I don’t want to wait till your patch passes Automatic QA, and code review, and additional rounds. I have no problem with the design you suggested me. However, this design can't work with the current trunk architecture, since in your design, shuffle.run() returns void and not iterator (you rely on merger to return the iterator). I suggest that you’ll continue with your patch on top of my patch. In case, you’ll need my help with the integration, I will be honored to assist. I am open to any idea that you or someone else may have. Thanks, Avner
          Hide
          Mariappan Asokan added a comment -

          I don’t have conflict of interests with you. 4 months ago, I already welcomed the watchers of this issue to help you commit your patch.

          I hope I didn't offend you by stating that "we seem to have a conflict of interest here." I meant the conflict in our designs

          RDMA is not coupled with any merge and there is no such a thing "RDMA merge".

          Perhaps I should have stated "RDMA based merge" instead of "RDMA merge." From my understanding of the pdf document attached to this Jira, there is a NetMerger plugin component which does the merge on the reduce side. It takes care of shuffling(small pieces of data using RDMA) and merge on the reduce side in order to avoid multi-phase merges and associated disk IO.

          If you can educate me and others on the relationship between NetMerger and your ShuffleConsumerPlugin, perhaps we will understand more. To me NetMerger implies it is doing a merge sort on the reduce side and hence it is qualified to be a sort plugin on the reduce side.

          It is current Hadoop that couples shuffle with merge. You can be relaxed. I don’t “want to retain that coupling”. My opinion is that your decoupling is correct idea and I encourage it.

          I am glad that you like the idea of decoupling the merge and shuffle. In order to achieve this decoupling, Shuffle class cannot return the key value iterator. It should only do the shuffling of data and feed it to the code that merges the map outputs. It is the responsibility of the merger to return the key value iterator. I want to make that code pluggable and hence the need for ReduceSortPlugin. If you want to make shuffle pluggable, as I suggested before we can make
          ShuffleRunner(which will be implemented by Shuffle class) pluggable.

          Your patch contains more than 7,000 rows, while my patch is only 400 rows. I don’t want to wait till your patch passes Automatic QA, and code review, and additional rounds.

          Since we are working on overlapping areas of the code, I just want to make sure that we both get the design concepts right before looking at the number of lines of code touched or which Jira is to be committed first and so on.

          Show
          Mariappan Asokan added a comment - I don’t have conflict of interests with you. 4 months ago, I already welcomed the watchers of this issue to help you commit your patch. I hope I didn't offend you by stating that "we seem to have a conflict of interest here." I meant the conflict in our designs RDMA is not coupled with any merge and there is no such a thing "RDMA merge". Perhaps I should have stated "RDMA based merge" instead of "RDMA merge." From my understanding of the pdf document attached to this Jira, there is a NetMerger plugin component which does the merge on the reduce side. It takes care of shuffling(small pieces of data using RDMA) and merge on the reduce side in order to avoid multi-phase merges and associated disk IO. If you can educate me and others on the relationship between NetMerger and your ShuffleConsumerPlugin , perhaps we will understand more. To me NetMerger implies it is doing a merge sort on the reduce side and hence it is qualified to be a sort plugin on the reduce side. It is current Hadoop that couples shuffle with merge. You can be relaxed. I don’t “want to retain that coupling”. My opinion is that your decoupling is correct idea and I encourage it. I am glad that you like the idea of decoupling the merge and shuffle. In order to achieve this decoupling, Shuffle class cannot return the key value iterator. It should only do the shuffling of data and feed it to the code that merges the map outputs. It is the responsibility of the merger to return the key value iterator. I want to make that code pluggable and hence the need for ReduceSortPlugin. If you want to make shuffle pluggable, as I suggested before we can make ShuffleRunner (which will be implemented by Shuffle class) pluggable. Your patch contains more than 7,000 rows, while my patch is only 400 rows. I don’t want to wait till your patch passes Automatic QA, and code review, and additional rounds. Since we are working on overlapping areas of the code, I just want to make sure that we both get the design concepts right before looking at the number of lines of code touched or which Jira is to be committed first and so on.
          Hide
          Avner BenHanoch added a comment -

          Asokan,
          My design has no conflict with your design. Below is a comment I wrote you 4 months ago:

          Your patch for the trunk is good enough for my needs. I can write my RDMA shuffle plugin based on either your patch or based on my patch. Hence, I am not planning to submit additional patch for the trunk on top of your patch. (I will only submit patch for 1.x)

          (I have only now submitted a patch for the trunk, because of Arun's/Todd's comment on my 1.x patch)

          The academic paper I pointed as Reference should not be confused with my plugin (Personally, I consider code in academic researches as POC and not as product). The two relevant conclusions I take from this academic research are:
          1) Hadoop can benefit from RDMA shuffle and shuffle plugin-ability
          2) With fast shuffle, Hadoop can benefit from additional merge algorithms that are not practical with slow shuffle.
          That's all! There is no request for Hadoop to keep its coupling of shuffle with merge.
          Again, I encourage your decoupling! When your patch will be accepted to the trunk, I will adjust future versions of my plugin following your decoupling.

          My design should not disturb you in any way!
          When reviewing my design from ReduceTask.java point of view, If you merely rename: ShuffleConsumerPlugin -> ReduceFeederPlugin, then you could easily see that your decoupling design can peacefully come after my design.
          I believe the thing that disturbs you is that currently Hadoop uses 'shuffle' which invokes 'merge' while you want the opposite direction. However, this is outside the scope of my patch. Hence, you are welcome to build your patch on top of mine. It is not really different than building your patch on top of the current trunk.
          I will be more than happy to assist you with anything you might need, and I'll appreciate it if you gave me your blessing for my commit

          Show
          Avner BenHanoch added a comment - Asokan, My design has no conflict with your design. Below is a comment I wrote you 4 months ago : Your patch for the trunk is good enough for my needs. I can write my RDMA shuffle plugin based on either your patch or based on my patch. Hence, I am not planning to submit additional patch for the trunk on top of your patch. (I will only submit patch for 1.x) (I have only now submitted a patch for the trunk, because of Arun's/Todd's comment on my 1.x patch) The academic paper I pointed as Reference should not be confused with my plugin (Personally, I consider code in academic researches as POC and not as product). The two relevant conclusions I take from this academic research are: 1) Hadoop can benefit from RDMA shuffle and shuffle plugin-ability 2) With fast shuffle, Hadoop can benefit from additional merge algorithms that are not practical with slow shuffle. That's all! There is no request for Hadoop to keep its coupling of shuffle with merge. Again, I encourage your decoupling! When your patch will be accepted to the trunk, I will adjust future versions of my plugin following your decoupling. My design should not disturb you in any way! When reviewing my design from ReduceTask.java point of view, If you merely rename: ShuffleConsumerPlugin -> ReduceFeederPlugin, then you could easily see that your decoupling design can peacefully come after my design. I believe the thing that disturbs you is that currently Hadoop uses 'shuffle' which invokes 'merge' while you want the opposite direction. However, this is outside the scope of my patch. Hence, you are welcome to build your patch on top of mine. It is not really different than building your patch on top of the current trunk. I will be more than happy to assist you with anything you might need, and I'll appreciate it if you gave me your blessing for my commit
          Hide
          Mariappan Asokan added a comment -

          Hi Avner,
          Thanks for the clarification. I am also back-porting MAPREDUCE-2454 to Hadoop 1.1(please see MAPREDUCE-4482.) There also, I am trying to decouple merge related code from ReduceCopier class in ReduceTask.java. I started my work initially on the trunk version because Arun asked me to do so. Once I finish refactoring ReduceCopier I will post a patch in MAPREDUCE-4482. Please take a look at it when I am done.

          Thanks for your offer to help me.

          – Asokan

          Show
          Mariappan Asokan added a comment - Hi Avner, Thanks for the clarification. I am also back-porting MAPREDUCE-2454 to Hadoop 1.1(please see MAPREDUCE-4482 .) There also, I am trying to decouple merge related code from ReduceCopier class in ReduceTask.java . I started my work initially on the trunk version because Arun asked me to do so. Once I finish refactoring ReduceCopier I will post a patch in MAPREDUCE-4482 . Please take a look at it when I am done. Thanks for your offer to help me. – Asokan
          Hide
          Avner BenHanoch added a comment -

          Cool. I wish you good luck with your issues. I am watching them for staying in the picture and for any question you may have.
          I understand you don't have obligation to my commit any more. Right?
          (Please let me know if you want the rename: ShuffleConsumerPlugin -> ReduceFeederPlugin, or any other name)

          Show
          Avner BenHanoch added a comment - Cool. I wish you good luck with your issues. I am watching them for staying in the picture and for any question you may have. I understand you don't have obligation to my commit any more. Right? (Please let me know if you want the rename: ShuffleConsumerPlugin -> ReduceFeederPlugin, or any other name)
          Hide
          Avner BenHanoch added a comment -

          [correcting typo:]

          Cool. I wish you good luck with your issues. I am watching them for staying in the picture and for any question you may have.
          I understand you don't have objection to my commit any more. Right?
          (Please let me know if you want the rename: ShuffleConsumerPlugin -> ReduceFeederPlugin, or any other name)

          Show
          Avner BenHanoch added a comment - [correcting typo:] Cool. I wish you good luck with your issues. I am watching them for staying in the picture and for any question you may have. I understand you don't have objection to my commit any more. Right? (Please let me know if you want the rename: ShuffleConsumerPlugin -> ReduceFeederPlugin, or any other name)
          Hide
          Mariappan Asokan added a comment -

          Hi Avner,
          You agree that ShuffleConsumerPlugin should be decoupled from merge. In that case, it should have nothing to do with RawKeyValueIterator. However in its current form the run() method in ShuffleConsumerPlugin returns RawkKeyValueIterator. From a design point, my objection is that the abstraction ShuffleConsumerPlugin is not capturing the concept it is intended for namely moving just raw bytes from map hosts to reduce hosts and nothing more.

          I would ask you to go back to my original suggestion to make ShuffleRunner pluggable. We can add an initialize() method there.

          – Asokan

          Show
          Mariappan Asokan added a comment - Hi Avner, You agree that ShuffleConsumerPlugin should be decoupled from merge. In that case, it should have nothing to do with RawKeyValueIterator . However in its current form the run() method in ShuffleConsumerPlugin returns RawkKeyValueIterator . From a design point, my objection is that the abstraction ShuffleConsumerPlugin is not capturing the concept it is intended for namely moving just raw bytes from map hosts to reduce hosts and nothing more. I would ask you to go back to my original suggestion to make ShuffleRunner pluggable. We can add an initialize() method there. – Asokan
          Hide
          Avner BenHanoch added a comment -

          Hi Asokan,

          • Everyone agrees that Shuffle should be decoupled from Merge. However, the coupling occurs in the implementation of the current Shuffle.run(). I think we clarified that my patch has nothing to do with that.
          • It is the current ReduceTask.run() and the current mapreduce.Reducer.Context that uses RawKeyValueIterator for moving merged records and not only "raw bytes". My patch didn't change that.

          Fixing the above claims is out of my issue's scope.

          Please, if you have a concrete comment about my design, be specific. Don't attribute me the current trunk situation (I don't deserve this big honor ).

          Avner

          Show
          Avner BenHanoch added a comment - Hi Asokan, Everyone agrees that Shuffle should be decoupled from Merge. However, the coupling occurs in the implementation of the current Shuffle.run() . I think we clarified that my patch has nothing to do with that. It is the current ReduceTask.run() and the current mapreduce.Reducer.Context that uses RawKeyValueIterator for moving merged records and not only "raw bytes". My patch didn't change that. Fixing the above claims is out of my issue's scope. Please, if you have a concrete comment about my design, be specific. Don't attribute me the current trunk situation (I don't deserve this big honor ). Avner
          Hide
          Avner BenHanoch added a comment -

          Asokan, Arun, and the other folks,

          My patch is ready in the system with +1 overall in Hadoop QA. I'll be happy if someone can review and approve my patch, or give me appropriate comments. I promise the patch is very short and straightforward.

          I can’t progress with the comments I got so far, because these comments seemed to be relevant to the trunk itself regardless of my patch.

          Kindly please let me know how to proceed. This is the first time I am submitting a patch.

          Thanks,
          Avner

          Show
          Avner BenHanoch added a comment - Asokan, Arun, and the other folks, My patch is ready in the system with +1 overall in Hadoop QA. I'll be happy if someone can review and approve my patch, or give me appropriate comments. I promise the patch is very short and straightforward. I can’t progress with the comments I got so far, because these comments seemed to be relevant to the trunk itself regardless of my patch. Kindly please let me know how to proceed. This is the first time I am submitting a patch. Thanks, Avner
          Hide
          Arun C Murthy added a comment -

          Avner - the patch looks reasonable - sorry, it took me so long to get around to it. I've been very busy with stabilizing & deploying YARN.

          Some minor comments:

          1. I'd ask you to you update the design doc with your latest approach, ideally also provide it as documentation patch to be checked in.
          2. I'd add a 'context' object to the consumer's init function rather than have 10+ input variables.
          Show
          Arun C Murthy added a comment - Avner - the patch looks reasonable - sorry, it took me so long to get around to it. I've been very busy with stabilizing & deploying YARN. Some minor comments: I'd ask you to you update the design doc with your latest approach, ideally also provide it as documentation patch to be checked in. I'd add a 'context' object to the consumer's init function rather than have 10+ input variables.
          Hide
          Avner BenHanoch added a comment -

          Hi Arun, Thanks for your feedback. I'll gladly handle your points.

          I need explanation regarding checking in the documentation. please direct me to the location to post such documentation file (and format if needed).

          Kindly thank you,
          Avner

          Show
          Avner BenHanoch added a comment - Hi Arun, Thanks for your feedback. I'll gladly handle your points. I need explanation regarding checking in the documentation. please direct me to the location to post such documentation file (and format if needed). Kindly thank you, Avner
          Hide
          Avner BenHanoch added a comment -

          Attached is the updated design documents.
          For your convenience, I am writing below its essence:

          • Support ShuffleConsumerPlugin from 3rd parties which will be able to retrieve maps from either the built-in ShuffleHandler or from a 3rd party Shuffle Provider Service (that can be loaded as an AuxiliaryService at run time)
          • Update Hadoop code, so the current Shuffle class will be the first (and the default) ShuffleConsumerPlugin.
          • Allow users to configure their desired ShuffleConsumerPlugin in mapred.xml files.
          • Change ReduceTask to dynamically load the user configured ShuffleConsumerPlugin and use it for its shuffle needs.
          • The interface of ShuffleConsumerPlugin will be inspired by the interface of the existing Shuffle class. Hence, there will be minimal changes in current code.
          ShuffleConsumerPlugin.java
          /**
           * ShuffleConsumerPlugin for serving Reducers.  It may shuffle MOF files from
           * either the built-in ShuffleHandler or from a 3rd party AuxiliaryService.
           */
          public abstract class ShuffleConsumerPlugin<K, V> {
          	
          
            public abstract void init(ShuffleContext<K, V> context);				
          
            public abstract RawKeyValueIterator run() throws IOException, InterruptedException;
          
            public void close(){}
          
            /**
             * Factory method for getting a ShuffleConsumerPlugin from the given class
             * object and configuring it.  If clazz is null, this method will return an
             * instance of 'Shuffle' class since it is the default ShuffleConsumerPlugin 
             * 
             * @param clazz class of the requested ShuffleConsumerPlugin
             * @param conf configure the plugin with this
             * @return an instance of ShuffleConsumerPlugin
             */
            public static ShuffleConsumerPlugin getShuffleConsumerPlugin(
              Class<? extends ShuffleConsumerPlugin> clazz, 
              JobConf conf) throws ClassNotFoundException, IOException {
          	// ...		
            }
          }
          
          Show
          Avner BenHanoch added a comment - Attached is the updated design documents. For your convenience, I am writing below its essence: Support ShuffleConsumerPlugin from 3rd parties which will be able to retrieve maps from either the built-in ShuffleHandler or from a 3rd party Shuffle Provider Service (that can be loaded as an AuxiliaryService at run time) Update Hadoop code, so the current Shuffle class will be the first (and the default) ShuffleConsumerPlugin . Allow users to configure their desired ShuffleConsumerPlugin in mapred.xml files. Change ReduceTask to dynamically load the user configured ShuffleConsumerPlugin and use it for its shuffle needs. The interface of ShuffleConsumerPlugin will be inspired by the interface of the existing Shuffle class. Hence, there will be minimal changes in current code. ShuffleConsumerPlugin.java /** * ShuffleConsumerPlugin for serving Reducers. It may shuffle MOF files from * either the built-in ShuffleHandler or from a 3rd party AuxiliaryService. */ public abstract class ShuffleConsumerPlugin<K, V> { public abstract void init(ShuffleContext<K, V> context); public abstract RawKeyValueIterator run() throws IOException, InterruptedException; public void close(){} /** * Factory method for getting a ShuffleConsumerPlugin from the given class * object and configuring it. If clazz is null , this method will return an * instance of 'Shuffle' class since it is the default ShuffleConsumerPlugin * * @param clazz class of the requested ShuffleConsumerPlugin * @param conf configure the plugin with this * @ return an instance of ShuffleConsumerPlugin */ public static ShuffleConsumerPlugin getShuffleConsumerPlugin( Class <? extends ShuffleConsumerPlugin> clazz, JobConf conf) throws ClassNotFoundException, IOException { // ... } }
          Hide
          Hadoop QA added a comment -

          +1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12552017/mapreduce-4049.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/2986//testReport/
          Console output: https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/2986//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/12552017/mapreduce-4049.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/2986//testReport/ Console output: https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/2986//console This message is automatically generated.
          Hide
          Hadoop QA added a comment -

          +1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12552079/mapreduce-4049.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/2987//testReport/
          Console output: https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/2987//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/12552079/mapreduce-4049.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/2987//testReport/ Console output: https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/2987//console This message is automatically generated.
          Hide
          Avner BenHanoch added a comment -

          Hi Arun,

          Following your request, I updated the design doc and I submitted a new patch in which I added a 'context' object to the consumer's init function.
          (If needed, I'll be glad to provide the design doc as documentation patch to be checked in; however, I don't know in which format/place to do that).

          I welcome your review.

          Cheers,
          Avner

          Show
          Avner BenHanoch added a comment - Hi Arun, Following your request, I updated the design doc and I submitted a new patch in which I added a 'context' object to the consumer's init function. (If needed, I'll be glad to provide the design doc as documentation patch to be checked in; however, I don't know in which format/place to do that). I welcome your review. Cheers, Avner
          Hide
          Arun C Murthy added a comment -

          Avner, the patch looks good. One nit: we use getConf() rather than conf() in our contexts. Mind fixing that? Tx.

          Show
          Arun C Murthy added a comment - Avner, the patch looks good. One nit: we use getConf() rather than conf() in our contexts. Mind fixing that? Tx.
          Hide
          Clinton Ooi added a comment -

          I am currently OOO on vacation from November 10th - November 25th with no email/cellphone access. For inquiries related to Analytics Workbench, please contact Tasneem Maistry at tashneem.maistry@emc.com. For any other inquiries, please contact my manager Apurva Desai at apurva.desai@emc.com

          Thank you.
          Clinton Ooi

          Show
          Clinton Ooi added a comment - I am currently OOO on vacation from November 10th - November 25th with no email/cellphone access. For inquiries related to Analytics Workbench, please contact Tasneem Maistry at tashneem.maistry@emc.com. For any other inquiries, please contact my manager Apurva Desai at apurva.desai@emc.com Thank you. Clinton Ooi
          Hide
          Alejandro Abdelnur added a comment -

          Looks good, but there are a few more things that should be addressed in the patch:

          • The Shuffle class should be renamed to DefaultShuffle.
          • The ShuffleConsumerPlugin should be renamed to Shuffle.
          • Checking for shuffleConsumerPlugin != null before closing it seems redundant, you would have never got there if shufflePlugin is NULL.
          • ShuffleConsumerPlugin, getShuffleConsumerPlugin() method is not required, instead use the ReflectionUtils directly in the ReducerTask class.
          • ReduceTask, the obtention of the ShuffleConsumerPlugin implementation should use the Shuffle class as default, instead of NULL.
          • Visibility annotations for the ShuffleConsumerPlugin, ShuffleContext, should be Unstable and LimitedPrivate( {"MapReduce"}. Shuffle should change from Private to LimitedPrivate({"MapReduce"}
          Show
          Alejandro Abdelnur added a comment - Looks good, but there are a few more things that should be addressed in the patch: The Shuffle class should be renamed to DefaultShuffle. The ShuffleConsumerPlugin should be renamed to Shuffle. Checking for shuffleConsumerPlugin != null before closing it seems redundant, you would have never got there if shufflePlugin is NULL. ShuffleConsumerPlugin, getShuffleConsumerPlugin() method is not required, instead use the ReflectionUtils directly in the ReducerTask class. ReduceTask, the obtention of the ShuffleConsumerPlugin implementation should use the Shuffle class as default, instead of NULL. Visibility annotations for the ShuffleConsumerPlugin, ShuffleContext, should be Unstable and LimitedPrivate( {"MapReduce"}. Shuffle should change from Private to LimitedPrivate({"MapReduce"}
          Hide
          Alejandro Abdelnur added a comment -

          Just realized after seeing MAPREDUCE-4812 that this JIRA is tightly related to MAPREDUCE-2454, thus made it a sub task of it.

          As all this JIRAs are small, I think we'll be able to move fast with all of them.

          Show
          Alejandro Abdelnur added a comment - Just realized after seeing MAPREDUCE-4812 that this JIRA is tightly related to MAPREDUCE-2454 , thus made it a sub task of it. As all this JIRAs are small, I think we'll be able to move fast with all of them.
          Hide
          Alejandro Abdelnur added a comment -

          Also, please lets hold off on committing this to branch-1 until we discuss that explicitly.

          Show
          Alejandro Abdelnur added a comment - Also, please lets hold off on committing this to branch-1 until we discuss that explicitly.
          Hide
          Alejandro Abdelnur added a comment - - edited

          A couple more comments.

          • Along the lines of the comment done in MAPREDUCE-4812, the ShuffleContext could be moved as public static inner class 'Context' in the Shuffle interface.
          • a more appropriate name for the configuration property for the shuffle plugin (and consistent with the other subtask in MAPREDUCE-2454) would be 'mapreduce.job.reduce.shuffle.class'
          Show
          Alejandro Abdelnur added a comment - - edited A couple more comments. Along the lines of the comment done in MAPREDUCE-4812 , the ShuffleContext could be moved as public static inner class 'Context' in the Shuffle interface. a more appropriate name for the configuration property for the shuffle plugin (and consistent with the other subtask in MAPREDUCE-2454 ) would be 'mapreduce.job.reduce.shuffle.class'
          Hide
          Laxman added a comment -

          Hi Avner, we are also impressed by "Network Levitated Merge" algorithm and we are in the process of implementing this. With your consent, we would like to collaborate and contribute to this issue.

          Show
          Laxman added a comment - Hi Avner, we are also impressed by "Network Levitated Merge" algorithm and we are in the process of implementing this. With your consent, we would like to collaborate and contribute to this issue.
          Hide
          Avner BenHanoch added a comment -

          Hi Laxman,

          Thanks for your comment and sorry for my late response.

          I just posted a link for downloading the source code of Mellanox plugin that implements generic shuffle using RDMA and levitated merge.

          You are warmly welcomed to contribute to push the algorithms of this plugin to the core of vanilla Hadoop, as well as to help accepting my straight forward patch in this JIRA issue.
          Avner

          Show
          Avner BenHanoch added a comment - Hi Laxman, Thanks for your comment and sorry for my late response. I just posted a link for downloading the source code of Mellanox plugin that implements generic shuffle using RDMA and levitated merge. You are warmly welcomed to contribute to push the algorithms of this plugin to the core of vanilla Hadoop, as well as to help accepting my straight forward patch in this JIRA issue. Avner
          Hide
          Avner BenHanoch added a comment -

          Alejandro,

          With all due respect, I think that something in your behavior is inappropriate:

          • You were never involved in this issue; still you gave yourself the liberty to make it a sub issue of your supported MAPREDUCE-2454 issue, without consulting anyone.
          • This is especially inappropriate since MAPREDUCE-2454 is disputable and has its acceptance problems regardless of my issue. Hence, its acceptance problems will affect my issue.
          • Your justification "As all this JIRAs are small, I think we'll be able to move fast with all of them." is inappropriate since you actually created a linkage that will surely postpone my issue instead of leaving each issue to progress at its own pace!
          • It is not the first time that the persons behind MAPREDUCE-2454 try to disturb this JIRA issue.

          Apparently, I don't have the privileges to break this "sub task" linkage; hence, I am asking that you or someone else will do it.

          I am welcoming any comment coming from a professional place with the simple target of making Hadoop better. Having that said, I feel that the way you blitzed my patch with any possible patty comment, sometime with disputable claims, just before the patch is about to be accepted – is unfair, unprofessional and unfriendly. Especially considering your complete silence since this JIRA issue has started.

          I am not sure that commenting in a blitz way will increase the quality of hadoop. For example:

          "Checking for shuffleConsumerPlugin != null before closing it seems redundant, you would have never got there if shufflePlugin is NULL."

          This is your mistake (I'll reach there in case isLocal == true). There is no option to remove the nullity check!

          "Visibility annotations for the ShuffleConsumerPlugin, ShuffleContext, should be Unstable"

          I think it is inappropriate to declare plugin interface as "Unstable", since it must stay stable for 3rd party vendors.

          — --- — ---

          Personally, I have no problem to implement all the rest of your comments. It should be very easy for me. Still, I am raising few points for consideration regarding your following comments:

          "The Shuffle class should be renamed to DefaultShuffle."
          "The ShuffleConsumerPlugin should be renamed to Shuffle."

          I chose the term 'ShuffleConsumerPlugin' and not something like 'Shuffle', because it clarifies that we are in a plugin of ShuffleConsumer, rather than a builtin ShuffleProvider/ShuffleHandler. Also, I didn't take the liberty to rename core classes of Hadoop.

          "ShuffleConsumerPlugin, getShuffleConsumerPlugin() method is not required, instead use the ReflectionUtils directly in the ReducerTask class."

          Here, I only followed existing convention of Hadoop as shown in ResourceCalculatorPlugin.getResourceCalculatorPlugin(). Personally, I'll be glad to follow your advice, and even to go one step further and make ShuffleConsumerPlugin an interface instead of AbstractClass.

          "use 'mapreduce.job.reduce.shuffle.class' to be consistent with MAPREDUCE-2454."

          Here I chose 'mapreduce.shuffle…', since I think it is consistent with the current convention in hadoop-3 configuration.

          — --- — ---

          I can tell you that Arun & Todd didn't make it easy for me with their requests from this patch so far. Still, I understand, respect and accept all their comments. I am sure that everyone involved only want the best for Hadoop.
          I suggest we hear Arun's consideration and move forward with the patch in the best professional way.

          Arun,
          I think you are very familiar with both Hadoop/MapReduce and this JIRA issue since its inception. You are also well familiar and involved with MAPREDUCE-2454. It is also safe to say you know Alejandro and Asokan better than you know me. I believe everyone involved will agree that your sole interest is Hadoop's quality. I am asking you and everyone else to help progressing here.

          Avner

          Show
          Avner BenHanoch added a comment - Alejandro, With all due respect, I think that something in your behavior is inappropriate: You were never involved in this issue; still you gave yourself the liberty to make it a sub issue of your supported MAPREDUCE-2454 issue, without consulting anyone. This is especially inappropriate since MAPREDUCE-2454 is disputable and has its acceptance problems regardless of my issue. Hence, its acceptance problems will affect my issue. Your justification "As all this JIRAs are small, I think we'll be able to move fast with all of them." is inappropriate since you actually created a linkage that will surely postpone my issue instead of leaving each issue to progress at its own pace! It is not the first time that the persons behind MAPREDUCE-2454 try to disturb this JIRA issue. Apparently, I don't have the privileges to break this "sub task" linkage; hence, I am asking that you or someone else will do it. I am welcoming any comment coming from a professional place with the simple target of making Hadoop better. Having that said, I feel that the way you blitzed my patch with any possible patty comment, sometime with disputable claims, just before the patch is about to be accepted – is unfair, unprofessional and unfriendly. Especially considering your complete silence since this JIRA issue has started. I am not sure that commenting in a blitz way will increase the quality of hadoop. For example: "Checking for shuffleConsumerPlugin != null before closing it seems redundant, you would have never got there if shufflePlugin is NULL." This is your mistake (I'll reach there in case isLocal == true). There is no option to remove the nullity check! "Visibility annotations for the ShuffleConsumerPlugin, ShuffleContext, should be Unstable" I think it is inappropriate to declare plugin interface as "Unstable", since it must stay stable for 3rd party vendors. — --- — --- Personally, I have no problem to implement all the rest of your comments. It should be very easy for me. Still, I am raising few points for consideration regarding your following comments: "The Shuffle class should be renamed to DefaultShuffle." "The ShuffleConsumerPlugin should be renamed to Shuffle." I chose the term 'ShuffleConsumerPlugin' and not something like 'Shuffle', because it clarifies that we are in a plugin of ShuffleConsumer , rather than a builtin ShuffleProvider/ShuffleHandler . Also, I didn't take the liberty to rename core classes of Hadoop. "ShuffleConsumerPlugin, getShuffleConsumerPlugin() method is not required, instead use the ReflectionUtils directly in the ReducerTask class." Here, I only followed existing convention of Hadoop as shown in ResourceCalculatorPlugin.getResourceCalculatorPlugin(). Personally, I'll be glad to follow your advice, and even to go one step further and make ShuffleConsumerPlugin an interface instead of AbstractClass. "use 'mapreduce.job.reduce.shuffle.class' to be consistent with MAPREDUCE-2454 ." Here I chose 'mapreduce.shuffle…', since I think it is consistent with the current convention in hadoop-3 configuration. — --- — --- I can tell you that Arun & Todd didn't make it easy for me with their requests from this patch so far. Still, I understand, respect and accept all their comments. I am sure that everyone involved only want the best for Hadoop. I suggest we hear Arun's consideration and move forward with the patch in the best professional way. Arun, I think you are very familiar with both Hadoop/MapReduce and this JIRA issue since its inception. You are also well familiar and involved with MAPREDUCE-2454 . It is also safe to say you know Alejandro and Asokan better than you know me. I believe everyone involved will agree that your sole interest is Hadoop's quality. I am asking you and everyone else to help progressing here. Avner
          Hide
          Laxman added a comment -

          You are warmly welcomed to contribute to push the algorithms of this plugin to the core of vanilla Hadoop

          Thank you Avner. I wish to see this as part of hadoop.
          I'm not able to build UDA you have provided as per BUILD.README provided in the downloaded bundle. SVN repository provided is not accessible/resolvable.

          https://sirius.voltaire.com/repos/enterprise/uda/trunk

          as well as to help accepting my straight forward patch in this JIRA issue.

          I will personally request few of my friends (Hadoop contributors) to review this jira.

          Show
          Laxman added a comment - You are warmly welcomed to contribute to push the algorithms of this plugin to the core of vanilla Hadoop Thank you Avner. I wish to see this as part of hadoop. I'm not able to build UDA you have provided as per BUILD.README provided in the downloaded bundle. SVN repository provided is not accessible/resolvable. https://sirius.voltaire.com/repos/enterprise/uda/trunk as well as to help accepting my straight forward patch in this JIRA issue. I will personally request few of my friends (Hadoop contributors) to review this jira.
          Hide
          Laxman added a comment -

          I'm trying to build as per the README available here (http://mellanox.com/downloads/UDA/UDA3.0_Release.tar.gz).

          Show
          Laxman added a comment - I'm trying to build as per the README available here ( http://mellanox.com/downloads/UDA/UDA3.0_Release.tar.gz ).
          Hide
          Avner BenHanoch added a comment -

          Hi Laxman,

          You are referring to an internal document (there is no external document yet ).
          The svn is only for downloading internally clean sources for releasing new version. However, you already got the sources and you don't need it.

          In fast, I think you should use:

          1. src/premake.sh
          2. build/makerpm.sh

          Also, in fast, Please expect compilation dependency:

          • In the C++, on librdmacm-devel
          • In the java, you'll need to copy the hadoop jars, that are used by the plugin, into the plugin's directory (see them according to CLASSPATH in the makefile at the plugin's directory)

          Before you go with the java side, you may choose to edit makerpm.sh and comment out hadoop flavors that you don't care about.

          Please be aware that you are the 1st one that tries to build the sources outside Mellanox.
          Also, I am not sure this is the place and way to get support for Mellanox products.

          Show
          Avner BenHanoch added a comment - Hi Laxman, You are referring to an internal document (there is no external document yet ). The svn is only for downloading internally clean sources for releasing new version. However, you already got the sources and you don't need it. In fast, I think you should use: src/premake.sh build/makerpm.sh Also, in fast, Please expect compilation dependency: In the C++, on librdmacm-devel In the java, you'll need to copy the hadoop jars, that are used by the plugin, into the plugin's directory (see them according to CLASSPATH in the makefile at the plugin's directory) Before you go with the java side, you may choose to edit makerpm.sh and comment out hadoop flavors that you don't care about. Please be aware that you are the 1st one that tries to build the sources outside Mellanox. Also, I am not sure this is the place and way to get support for Mellanox products.
          Hide
          Alejandro Abdelnur added a comment -

          Hi Avner,

          I respectfully disagree with your opinion that my behavior is inappropriate.

          First of all, it is not my intention to slow you this JIRA down, but to make sure it is consistent with the related work in MAPREDUCE-2454 (you can see that in my comments). If that requires a couple of extra days, is is a small price to pay.

          As an Apache Hadoop developer is my responsibility to review and provide feedback on work posted by other developers, my usual triggers are area of knowledge, related work and area of interest.

          This JIRA is tightly related to MAPREDUCE-2454, there is not dispute on that. Thus it should stay as a subtask of it.

          MAPREDUCE-2454 is not disputable, as it has been commented in it JIRA, it is almost ready, it was matter of breaking it up and doing an fast interactive review of its parts. As far as I can tell, this is already happening there.

          Now going to your comments on my review:

          • Yes the shuffleConsumerPlugin != null, you are right, I've noticed that after I've posted my comments, so you can disregard that done.
          • On the marking the ShuffleConsumerPlugin, ShuffleContext as unstable, it is not appropriate, Hadoop wants to keep the right of modifying these APIs in the future, if hte need arises. You can also see this, no only in MAPREDUCE-2454, but in several places where Hadoop provides pluggability (ie ResourceManagement, authentication).
          • On making the ShuffleConsumerPlugin and interface, that is a good idea, it will align things with the other sub-tasks.

          Looking forward to see the updated patch.

          Cheers.

          Show
          Alejandro Abdelnur added a comment - Hi Avner, I respectfully disagree with your opinion that my behavior is inappropriate. First of all, it is not my intention to slow you this JIRA down, but to make sure it is consistent with the related work in MAPREDUCE-2454 (you can see that in my comments). If that requires a couple of extra days, is is a small price to pay. As an Apache Hadoop developer is my responsibility to review and provide feedback on work posted by other developers, my usual triggers are area of knowledge, related work and area of interest. This JIRA is tightly related to MAPREDUCE-2454 , there is not dispute on that. Thus it should stay as a subtask of it. MAPREDUCE-2454 is not disputable, as it has been commented in it JIRA, it is almost ready, it was matter of breaking it up and doing an fast interactive review of its parts. As far as I can tell, this is already happening there. Now going to your comments on my review: Yes the shuffleConsumerPlugin != null , you are right, I've noticed that after I've posted my comments, so you can disregard that done. On the marking the ShuffleConsumerPlugin, ShuffleContext as unstable , it is not appropriate, Hadoop wants to keep the right of modifying these APIs in the future, if hte need arises. You can also see this, no only in MAPREDUCE-2454 , but in several places where Hadoop provides pluggability (ie ResourceManagement, authentication). On making the ShuffleConsumerPlugin and interface, that is a good idea, it will align things with the other sub-tasks. Looking forward to see the updated patch. Cheers.
          Hide
          Arun C Murthy added a comment -

          Sorry, just caught up on this since I'm dealing with some health issues at home.

          Frankly, worrying about whose work is a subset of whose is a pointless exercise. Having said that, making related tasks sub-tasks makes sense as long as there is a coherent community (one or more developers) working together makes sense, I don't see it for MAPREDUCE-4049 vis-a-vis MAPREDUCE-2454.

          IAC, there is no need to debate this further - it's just a time sink.

          Finally, MAPREDUCE-2454 is a bunch of large-scale changes. I'm happy to commit this as long as it's ready to, without tying it in.


          Overall, I really don't like to see us egregiously rename core MR classes - at best it's pointless for private apis, and at worst it hammers svn log. So, pls do not change existing Shuffle etc.

          Avner, please upload a patch with other changes:

          1. Use @LimitedPrivate, that way it makes it clear that this is for implementers and not end-users.
          2. I'm ok with suggested config names (again, I'm not religious about naming).

          With that it's good to go.

          Show
          Arun C Murthy added a comment - Sorry, just caught up on this since I'm dealing with some health issues at home. Frankly, worrying about whose work is a subset of whose is a pointless exercise. Having said that, making related tasks sub-tasks makes sense as long as there is a coherent community (one or more developers) working together makes sense, I don't see it for MAPREDUCE-4049 vis-a-vis MAPREDUCE-2454 . IAC, there is no need to debate this further - it's just a time sink. Finally, MAPREDUCE-2454 is a bunch of large-scale changes. I'm happy to commit this as long as it's ready to, without tying it in. Overall, I really don't like to see us egregiously rename core MR classes - at best it's pointless for private apis, and at worst it hammers svn log. So, pls do not change existing Shuffle etc. Avner, please upload a patch with other changes: Use @LimitedPrivate, that way it makes it clear that this is for implementers and not end-users. I'm ok with suggested config names (again, I'm not religious about naming). With that it's good to go.
          Hide
          Avner BenHanoch added a comment -

          Arun & Alejandro - thanks for your clarifications!

          To summarize, I’ll implement all your comments, with the following emphasizes:

          • Leave class names as is: both for Shuffle and ShuffleConsumerPlugin.
          • ShuffleConsumerPlugin will be interface instead of AbstractClass
          • The property name will be mapreduce.job.reduce.shuffle.class
          • ShuffleConsumerPlugin & ShuffleContext need to be @LimitedPrivate (without @unstable since it is interface for 3rd party vendors)

          Please ACK!

          Show
          Avner BenHanoch added a comment - Arun & Alejandro - thanks for your clarifications! To summarize, I’ll implement all your comments, with the following emphasizes: Leave class names as is: both for Shuffle and ShuffleConsumerPlugin . ShuffleConsumerPlugin will be interface instead of AbstractClass The property name will be mapreduce.job.reduce.shuffle.class ShuffleConsumerPlugin & ShuffleContext need to be @LimitedPrivate (without @unstable since it is interface for 3rd party vendors) Please ACK!
          Hide
          Alejandro Abdelnur added a comment -

          (Arun, hopefully you family health issues are on the right track)

          Avner,

          • I'm ok with leaving Shuffle as it is, though I don't like the Consumer in ShufleConsumerPlugin interface, I'd be OK with ShufflePlugin.
          • The property name should relfect the final name o the ShufleConsumerPlugin interface.
          • Please make ShuffleContext a static inner class of the ShufleConsumerPlugin interface called Context.

          While I'm not religious about names, I do care. In this case, we have the opportunity to have a consistent set of names and APIs (ie inner Context) for a set of related plugins (all the ones affected by MAPREDUCE-2454).

          Show
          Alejandro Abdelnur added a comment - (Arun, hopefully you family health issues are on the right track) Avner, I'm ok with leaving Shuffle as it is, though I don't like the Consumer in ShufleConsumerPlugin interface, I'd be OK with ShufflePlugin . The property name should relfect the final name o the ShufleConsumerPlugin interface. Please make ShuffleContext a static inner class of the ShufleConsumerPlugin interface called Context . While I'm not religious about names, I do care. In this case, we have the opportunity to have a consistent set of names and APIs (ie inner Context) for a set of related plugins (all the ones affected by MAPREDUCE-2454 ).
          Hide
          Avner BenHanoch added a comment -

          Thanks Alejandro,

          I'll submit the patch next week, based on ALL your (and Arun's) comments:

          • Shuffle - leave the class name as is
          • ShufflePlugin - instead of ShuffleConsumerPlugin
          • ShufflePlugin will be an interface
          • property name will be: mapreduce.job.reduce.shuffle.plugin.class (Kindly let me know ASAP if you prefer other name, or in case you consulted mapred-default.xml and preferred names like mapreduce.reduce.shuffle... OR mapreduce.shuffle... )
          • ShuffleContext -> ShufflePlugin.Context - a static inner class
          • ShufflePlugin will be @LimitedPrivate (without @unstable)

          Cheers,
          Avner

          Show
          Avner BenHanoch added a comment - Thanks Alejandro, I'll submit the patch next week, based on ALL your (and Arun's) comments: Shuffle - leave the class name as is ShufflePlugin - instead of ShuffleConsumerPlugin ShufflePlugin will be an interface property name will be: mapreduce.job.reduce.shuffle.plugin.class (Kindly let me know ASAP if you prefer other name, or in case you consulted mapred-default.xml and preferred names like mapreduce.reduce.shuffle... OR mapreduce.shuffle... ) ShuffleContext -> ShufflePlugin.Context - a static inner class ShufflePlugin will be @LimitedPrivate (without @unstable) Cheers, Avner
          Hide
          Alejandro Abdelnur added a comment -

          Avner, everything looks good except your last bullet, ShufflePlugin & Context must be marked as @LimitedPrivate for MapReduce and as @Unstable.

          Show
          Alejandro Abdelnur added a comment - Avner, everything looks good except your last bullet, ShufflePlugin & Context must be marked as @LimitedPrivate for MapReduce and as @Unstable.
          Hide
          Alejandro Abdelnur added a comment -

          Avner, one more thing, please make sure the patch applies to branch MR-2454 (https://svn.apache.org/repos/asf/hadoop/common/branches/MR-2454). thx

          Show
          Alejandro Abdelnur added a comment - Avner, one more thing, please make sure the patch applies to branch MR-2454 ( https://svn.apache.org/repos/asf/hadoop/common/branches/MR-2454 ). thx
          Hide
          Arun C Murthy added a comment -

          Looks good.

          Some more I've noted previously:

          1. Context should have get/set apis
          2. I don't see a need to replace all member fields in Shuffle.java, just init them from the passed-in context.
          Show
          Arun C Murthy added a comment - Looks good. Some more I've noted previously: Context should have get/set apis I don't see a need to replace all member fields in Shuffle.java, just init them from the passed-in context.
          Hide
          Alejandro Abdelnur added a comment -

          Hey Arun, changing the Context methods to get*() makes sense. Adding set*() methods is not needed at this point, right?

          Show
          Alejandro Abdelnur added a comment - Hey Arun, changing the Context methods to get*() makes sense. Adding set*() methods is not needed at this point, right?
          Hide
          Avner BenHanoch added a comment -

          Arun/Alejandro, can you pls delink it?

          Show
          Avner BenHanoch added a comment - Arun/Alejandro, can you pls delink it?
          Hide
          Alejandro Abdelnur added a comment -

          Delink? you me remove it as sub-task?, If so, I'd like it to stay as subtask as they are related. Thx

          Show
          Alejandro Abdelnur added a comment - Delink? you me remove it as sub-task?, If so, I'd like it to stay as subtask as they are related. Thx
          Hide
          Alejandro Abdelnur added a comment -

          And don't worry about begin a subtask delaying it, I'll review it as soon as you post a patch and committed it when ready. The same is happening with the other subtasks, so things should be in quite quickly. Thx

          Show
          Alejandro Abdelnur added a comment - And don't worry about begin a subtask delaying it, I'll review it as soon as you post a patch and committed it when ready. The same is happening with the other subtasks, so things should be in quite quickly. Thx
          Hide
          Arun C Murthy added a comment -

          Alejandro - there seems to be some lingering history between the protagonists here and in MAPREDUCE-2454.

          There is no point trying to force each upon the other.

          Since it's different people working on it (who don't the same horizon) let's de-link them and take off ferrets, ok?

          Show
          Arun C Murthy added a comment - Alejandro - there seems to be some lingering history between the protagonists here and in MAPREDUCE-2454 . There is no point trying to force each upon the other. Since it's different people working on it (who don't the same horizon) let's de-link them and take off ferrets, ok?
          Hide
          Arun C Murthy added a comment -

          Avner, I can't seem to make you a 'contributor' and assign this jira to you. Some weird issue with JIRA, fyi.

          Show
          Arun C Murthy added a comment - Avner, I can't seem to make you a 'contributor' and assign this jira to you. Some weird issue with JIRA, fyi.
          Hide
          Alejandro Abdelnur added a comment - - edited

          Arun, as I said before, the works is related thus it should be done together. If there was some "lingering history", this seems to be in the past because now there is full synergy between the work done in the different JIRAs. We are s community, we have disagreements and we address them, this is how we suppose to work.

          Avner, just sorted out the JIRA glitch, and assigned the JIRA to you.

          Show
          Alejandro Abdelnur added a comment - - edited Arun, as I said before, the works is related thus it should be done together. If there was some "lingering history", this seems to be in the past because now there is full synergy between the work done in the different JIRAs. We are s community, we have disagreements and we address them, this is how we suppose to work. Avner, just sorted out the JIRA glitch, and assigned the JIRA to you.
          Hide
          Avner BenHanoch added a comment -

          Hi Arun,

          Kindly thank you for all the help.

          Regarding set methods in ShuffleContext. Do we really need that. As I see it, all members are only inited at once in the CTOR and never changed again. As a result I defined them all as private final.
          Hence, please clarify if you ask me to remove the final, and provide set methods instead?

          ( In case you prefer the current private final style, please let me know if you still want your other comment regarding un-replacing member fields in Shuffle.java )

          Thanks,
          Avner

          Show
          Avner BenHanoch added a comment - Hi Arun, Kindly thank you for all the help. Regarding set methods in ShuffleContext. Do we really need that. As I see it, all members are only inited at once in the CTOR and never changed again. As a result I defined them all as private final . Hence, please clarify if you ask me to remove the final , and provide set methods instead? ( In case you prefer the current private final style, please let me know if you still want your other comment regarding un-replacing member fields in Shuffle.java ) Thanks, Avner
          Hide
          Arun C Murthy added a comment -

          Sorry, didn't mean to imply we need setters, I was just saying we use getX/setX convention - hence a getConf() or a getFoo() suffices in this case.

          Show
          Arun C Murthy added a comment - Sorry, didn't mean to imply we need setters, I was just saying we use getX/setX convention - hence a getConf() or a getFoo() suffices in this case.
          Hide
          Alejandro Abdelnur added a comment -

          It seems that by mistake this JIRA was removed as subtask for MAPREDUCE-2454, adding it again.

          Show
          Alejandro Abdelnur added a comment - It seems that by mistake this JIRA was removed as subtask for MAPREDUCE-2454 , adding it again.
          Hide
          Hadoop QA added a comment -

          +1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12556383/mapreduce-4049.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/3103//testReport/
          Console output: https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/3103//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/12556383/mapreduce-4049.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/3103//testReport/ Console output: https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/3103//console This message is automatically generated.
          Hide
          Avner BenHanoch added a comment -

          Arun,

          Please review my patch. It fulfills all requests, including:

          • getXXX methods
          • ShuffleConsumerPlugin is interface
          • ShuffleConsumerPlugin.Context - public static inner
          • both classes are Unstable/LimitedPrivate
          • restored Shuffle members
          • the obtention of the ShuffleConsumerPlugin implementation uses the Shuffle class as default
          • property name is: mapreduce.job.reduce.shuffle.consumer.plugin.class

          Thanks for all,
          Avner

          Show
          Avner BenHanoch added a comment - Arun, Please review my patch. It fulfills all requests, including: getXXX methods ShuffleConsumerPlugin is interface ShuffleConsumerPlugin.Context - public static inner both classes are Unstable/LimitedPrivate restored Shuffle members the obtention of the ShuffleConsumerPlugin implementation uses the Shuffle class as default property name is: mapreduce.job.reduce.shuffle.consumer.plugin.class Thanks for all, Avner
          Hide
          Alejandro Abdelnur added a comment -

          Avner,

          Patch looks good, only NIT is that the mapred-default.xml does have the value as default Shuffle impl empty, it should be the Shuffle class. Then you can remove the following line:

          shuffleConsumerPlugin = (clazz != null) ? ReflectionUtils.newInstance(clazz, job) : new Shuffle();
          

          Thx, good job.

          Show
          Alejandro Abdelnur added a comment - Avner, Patch looks good, only NIT is that the mapred-default.xml does have the value as default Shuffle impl empty, it should be the Shuffle class. Then you can remove the following line: shuffleConsumerPlugin = (clazz != null ) ? ReflectionUtils.newInstance(clazz, job) : new Shuffle(); Thx, good job.
          Hide
          Alejandro Abdelnur added a comment -

          +1 pending jenkins.

          Show
          Alejandro Abdelnur added a comment - +1 pending jenkins.
          Hide
          Avner BenHanoch added a comment -

          Thanks for the fast code review!
          This patch should go to the trunk, as it should not be an hostage of other huge patches.

          Show
          Avner BenHanoch added a comment - Thanks for the fast code review! This patch should go to the trunk, as it should not be an hostage of other huge 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/12559764/mapreduce-4049.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/3104//testReport/
          Console output: https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/3104//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/12559764/mapreduce-4049.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/3104//testReport/ Console output: https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/3104//console This message is automatically generated.
          Hide
          Alejandro Abdelnur added a comment -

          Avner, as I've responded to you by email "As I've mentioned in this JIRA before, this is closely related to MR-2454. As MR-2454 is done in a branch, it would go in the branch first, and then merged into trunk. I expect this would happen fast, by the end of next week if no surprises arise."

          Show
          Alejandro Abdelnur added a comment - Avner, as I've responded to you by email "As I've mentioned in this JIRA before, this is closely related to MR-2454. As MR-2454 is done in a branch, it would go in the branch first, and then merged into trunk. I expect this would happen fast, by the end of next week if no surprises arise."
          Hide
          Avner BenHanoch added a comment -

          true - you responded.
          Still- we disagree.
          In my eyes this patch stands for itself and should not be delayed.

          Show
          Avner BenHanoch added a comment - true - you responded. Still- we disagree. In my eyes this patch stands for itself and should not be delayed.
          Hide
          Alejandro Abdelnur added a comment -

          Thanks Avner. Committed to branch MR-2454.

          Arun, as you've given the OK before and the changes where of form only, I've committed so Asokan can continue with the other sub-tasks. If any corrections need to be done we can do this before the merge to trunk.

          Show
          Alejandro Abdelnur added a comment - Thanks Avner. Committed to branch MR-2454. Arun, as you've given the OK before and the changes where of form only, I've committed so Asokan can continue with the other sub-tasks. If any corrections need to be done we can do this before the merge to trunk.
          Hide
          Arun C Murthy added a comment -

          Merged to trunk too.

          Show
          Arun C Murthy added a comment - Merged to trunk too.
          Hide
          Hudson added a comment -

          Integrated in Hadoop-trunk-Commit #3094 (See https://builds.apache.org/job/Hadoop-trunk-Commit/3094/)
          MAPREDUCE-4049. Experimental api to allow for alternate shuffle plugins. Contributed by Anver BenHanoch. (Revision 1418173)

          Result = SUCCESS
          acmurthy : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1418173
          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/ReduceTask.java
          • /hadoop/common/trunk/hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-core/src/main/java/org/apache/hadoop/mapred/ShuffleConsumerPlugin.java
          • /hadoop/common/trunk/hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-core/src/main/java/org/apache/hadoop/mapreduce/MRConfig.java
          • /hadoop/common/trunk/hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-core/src/main/java/org/apache/hadoop/mapreduce/task/reduce/Shuffle.java
          • /hadoop/common/trunk/hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-core/src/main/resources/mapred-default.xml
          • /hadoop/common/trunk/hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-core/src/test/java/org/apache/hadoop/mapreduce/TestShufflePlugin.java
          Show
          Hudson added a comment - Integrated in Hadoop-trunk-Commit #3094 (See https://builds.apache.org/job/Hadoop-trunk-Commit/3094/ ) MAPREDUCE-4049 . Experimental api to allow for alternate shuffle plugins. Contributed by Anver BenHanoch. (Revision 1418173) Result = SUCCESS acmurthy : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1418173 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/ReduceTask.java /hadoop/common/trunk/hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-core/src/main/java/org/apache/hadoop/mapred/ShuffleConsumerPlugin.java /hadoop/common/trunk/hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-core/src/main/java/org/apache/hadoop/mapreduce/MRConfig.java /hadoop/common/trunk/hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-core/src/main/java/org/apache/hadoop/mapreduce/task/reduce/Shuffle.java /hadoop/common/trunk/hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-core/src/main/resources/mapred-default.xml /hadoop/common/trunk/hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-core/src/test/java/org/apache/hadoop/mapreduce/TestShufflePlugin.java
          Hide
          Alejandro Abdelnur added a comment -

          Arun, please revert commit from trunk. These set of patches are related, and you created a branch for them. Once the work in the branch is ready, and you said before the work is quite close to be ready, we'll merge the branch to trunk. Thanks.

          Show
          Alejandro Abdelnur added a comment - Arun, please revert commit from trunk. These set of patches are related, and you created a branch for them. Once the work in the branch is ready, and you said before the work is quite close to be ready, we'll merge the branch to trunk. Thanks.
          Hide
          Alejandro Abdelnur added a comment -

          Arun,

          As per our phone conversation on WED, please correct me if I'm wrong:

          You indicated that your concerns with the changes introduced by MAPREDUCE-2454 (parent task of MAPREDUCE-4809, MAPREDUCE-4807, MAPREDUCE-4808, MAPREDUCE-4812, MAPREDUCE-4049) were:

          • Adding new interfaces to Hadoop that we'll later have to support.
          • Performance impact of these changes.

          Regarding your first concern, in the current form all these patches create interfaces around the natural functional boundaries of the currently hardcoded classes. Because of that I don't think there is a high risk here. The fixes proposed for MAPREDUCE-4842 do not alter these interface, thus enforcing the theory the chosen interfaces seem right. We still are annotating them as LimitedPrivate and Unstable, thus any body implementing them understands they could have to make changes between release of Hadoop.

          Regarding the performance impact of these changes, These JIRAs are changing hard coded 'new' instantiations with 'ReflectionUtils.newInstance()' calls. And as in the current code, all this happens at setup time, they don't happen during the shuffle.

          You said you would like to have gridmix runs to confirm with facts that there is no performance impact.

          Also you indicated that you have some comments on MAPREDUCE-4812 and MAPREDUCE-4808 and once they are addressed you are good.

          I know you have put a HUGE amount of effort reviewing this patches, as they are in the core of MAPREDUCE, I did so.

          These JIRAs have been sitting around for long already and quite a few people have manifested their interest of having them in the next Hadoop release.

          My proposal of work to move forward quickly with this set of patches, and I think I'm just echoing your ideas:

          • 1 Complete the work in the branch, so we can see the whole picture regarding interfaces being added to Hadoop.
          • 2 Run a gridmix on trunk and on the branch to see if there is any performance impact. And if there any, decide if it is acceptable or not.

          To move forward with #1, it would be great if you provide the feedback you said already have for MAPREDUCE-4812 and MAPREDUCE-4808.

          To move forward with #2, I'll set up a cluster with a reasonable number of nodes and run gridmix with and without the patch.

          So again, please revert the commit of MAPREDUCE-4049 from trunk until we do the outlined work.

          Show
          Alejandro Abdelnur added a comment - Arun, As per our phone conversation on WED, please correct me if I'm wrong: You indicated that your concerns with the changes introduced by MAPREDUCE-2454 (parent task of MAPREDUCE-4809 , MAPREDUCE-4807 , MAPREDUCE-4808 , MAPREDUCE-4812 , MAPREDUCE-4049 ) were: Adding new interfaces to Hadoop that we'll later have to support. Performance impact of these changes. Regarding your first concern, in the current form all these patches create interfaces around the natural functional boundaries of the currently hardcoded classes. Because of that I don't think there is a high risk here. The fixes proposed for MAPREDUCE-4842 do not alter these interface, thus enforcing the theory the chosen interfaces seem right. We still are annotating them as LimitedPrivate and Unstable, thus any body implementing them understands they could have to make changes between release of Hadoop. Regarding the performance impact of these changes, These JIRAs are changing hard coded 'new' instantiations with 'ReflectionUtils.newInstance()' calls. And as in the current code, all this happens at setup time, they don't happen during the shuffle. You said you would like to have gridmix runs to confirm with facts that there is no performance impact. Also you indicated that you have some comments on MAPREDUCE-4812 and MAPREDUCE-4808 and once they are addressed you are good. I know you have put a HUGE amount of effort reviewing this patches, as they are in the core of MAPREDUCE, I did so. These JIRAs have been sitting around for long already and quite a few people have manifested their interest of having them in the next Hadoop release. My proposal of work to move forward quickly with this set of patches, and I think I'm just echoing your ideas: 1 Complete the work in the branch, so we can see the whole picture regarding interfaces being added to Hadoop. 2 Run a gridmix on trunk and on the branch to see if there is any performance impact. And if there any, decide if it is acceptable or not. To move forward with #1, it would be great if you provide the feedback you said already have for MAPREDUCE-4812 and MAPREDUCE-4808 . To move forward with #2, I'll set up a cluster with a reasonable number of nodes and run gridmix with and without the patch. So again, please revert the commit of MAPREDUCE-4049 from trunk until we do the outlined work.
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Yarn-trunk #58 (See https://builds.apache.org/job/Hadoop-Yarn-trunk/58/)
          MAPREDUCE-4049. Experimental api to allow for alternate shuffle plugins. Contributed by Anver BenHanoch. (Revision 1418173)

          Result = SUCCESS
          acmurthy : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1418173
          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/ReduceTask.java
          • /hadoop/common/trunk/hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-core/src/main/java/org/apache/hadoop/mapred/ShuffleConsumerPlugin.java
          • /hadoop/common/trunk/hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-core/src/main/java/org/apache/hadoop/mapreduce/MRConfig.java
          • /hadoop/common/trunk/hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-core/src/main/java/org/apache/hadoop/mapreduce/task/reduce/Shuffle.java
          • /hadoop/common/trunk/hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-core/src/main/resources/mapred-default.xml
          • /hadoop/common/trunk/hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-core/src/test/java/org/apache/hadoop/mapreduce/TestShufflePlugin.java
          Show
          Hudson added a comment - Integrated in Hadoop-Yarn-trunk #58 (See https://builds.apache.org/job/Hadoop-Yarn-trunk/58/ ) MAPREDUCE-4049 . Experimental api to allow for alternate shuffle plugins. Contributed by Anver BenHanoch. (Revision 1418173) Result = SUCCESS acmurthy : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1418173 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/ReduceTask.java /hadoop/common/trunk/hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-core/src/main/java/org/apache/hadoop/mapred/ShuffleConsumerPlugin.java /hadoop/common/trunk/hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-core/src/main/java/org/apache/hadoop/mapreduce/MRConfig.java /hadoop/common/trunk/hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-core/src/main/java/org/apache/hadoop/mapreduce/task/reduce/Shuffle.java /hadoop/common/trunk/hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-core/src/main/resources/mapred-default.xml /hadoop/common/trunk/hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-core/src/test/java/org/apache/hadoop/mapreduce/TestShufflePlugin.java
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Hdfs-trunk #1247 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk/1247/)
          MAPREDUCE-4049. Experimental api to allow for alternate shuffle plugins. Contributed by Anver BenHanoch. (Revision 1418173)

          Result = FAILURE
          acmurthy : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1418173
          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/ReduceTask.java
          • /hadoop/common/trunk/hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-core/src/main/java/org/apache/hadoop/mapred/ShuffleConsumerPlugin.java
          • /hadoop/common/trunk/hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-core/src/main/java/org/apache/hadoop/mapreduce/MRConfig.java
          • /hadoop/common/trunk/hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-core/src/main/java/org/apache/hadoop/mapreduce/task/reduce/Shuffle.java
          • /hadoop/common/trunk/hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-core/src/main/resources/mapred-default.xml
          • /hadoop/common/trunk/hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-core/src/test/java/org/apache/hadoop/mapreduce/TestShufflePlugin.java
          Show
          Hudson added a comment - Integrated in Hadoop-Hdfs-trunk #1247 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk/1247/ ) MAPREDUCE-4049 . Experimental api to allow for alternate shuffle plugins. Contributed by Anver BenHanoch. (Revision 1418173) Result = FAILURE acmurthy : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1418173 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/ReduceTask.java /hadoop/common/trunk/hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-core/src/main/java/org/apache/hadoop/mapred/ShuffleConsumerPlugin.java /hadoop/common/trunk/hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-core/src/main/java/org/apache/hadoop/mapreduce/MRConfig.java /hadoop/common/trunk/hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-core/src/main/java/org/apache/hadoop/mapreduce/task/reduce/Shuffle.java /hadoop/common/trunk/hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-core/src/main/resources/mapred-default.xml /hadoop/common/trunk/hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-core/src/test/java/org/apache/hadoop/mapreduce/TestShufflePlugin.java
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Mapreduce-trunk #1278 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk/1278/)
          MAPREDUCE-4049. Experimental api to allow for alternate shuffle plugins. Contributed by Anver BenHanoch. (Revision 1418173)

          Result = SUCCESS
          acmurthy : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1418173
          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/ReduceTask.java
          • /hadoop/common/trunk/hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-core/src/main/java/org/apache/hadoop/mapred/ShuffleConsumerPlugin.java
          • /hadoop/common/trunk/hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-core/src/main/java/org/apache/hadoop/mapreduce/MRConfig.java
          • /hadoop/common/trunk/hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-core/src/main/java/org/apache/hadoop/mapreduce/task/reduce/Shuffle.java
          • /hadoop/common/trunk/hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-core/src/main/resources/mapred-default.xml
          • /hadoop/common/trunk/hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-core/src/test/java/org/apache/hadoop/mapreduce/TestShufflePlugin.java
          Show
          Hudson added a comment - Integrated in Hadoop-Mapreduce-trunk #1278 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk/1278/ ) MAPREDUCE-4049 . Experimental api to allow for alternate shuffle plugins. Contributed by Anver BenHanoch. (Revision 1418173) Result = SUCCESS acmurthy : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1418173 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/ReduceTask.java /hadoop/common/trunk/hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-core/src/main/java/org/apache/hadoop/mapred/ShuffleConsumerPlugin.java /hadoop/common/trunk/hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-core/src/main/java/org/apache/hadoop/mapreduce/MRConfig.java /hadoop/common/trunk/hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-core/src/main/java/org/apache/hadoop/mapreduce/task/reduce/Shuffle.java /hadoop/common/trunk/hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-core/src/main/resources/mapred-default.xml /hadoop/common/trunk/hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-core/src/test/java/org/apache/hadoop/mapreduce/TestShufflePlugin.java
          Hide
          Arun C Murthy added a comment -

          Alejandro Abdelnur I'm confused.

          MAPREDUCE-2454 was a huge patch by a different contributor which got broken up to aid through reviews.

          MAPREDUCE-4049 is, effectively, a trivial patch from another contributor after he (Avner) has very patiently taken in all feedback. We should be thankful.

          I don't see why we need to block Avner's work on MAPREDUCE-2454. Furthermore, Avner has made it crystal clear that he has issues working with people working on MAPREDUCE-2454 (see http://s.apache.org/MRT, http://s.apache.org/6bh, http://s.apache.org/fR4). Why coerce him?

          Do you have any technical reason to revert the commit? Else, we can close this discussion.

          Show
          Arun C Murthy added a comment - Alejandro Abdelnur I'm confused. MAPREDUCE-2454 was a huge patch by a different contributor which got broken up to aid through reviews. MAPREDUCE-4049 is, effectively, a trivial patch from another contributor after he (Avner) has very patiently taken in all feedback. We should be thankful. I don't see why we need to block Avner's work on MAPREDUCE-2454 . Furthermore, Avner has made it crystal clear that he has issues working with people working on MAPREDUCE-2454 (see http://s.apache.org/MRT , http://s.apache.org/6bh , http://s.apache.org/fR4 ). Why coerce him? Do you have any technical reason to revert the commit? Else, we can close this discussion.
          Hide
          Alejandro Abdelnur added a comment -

          Arun, focusing on the technical side of your comments. My reasons to revert the patch from trunk are:

          All these components are highly interrelated as you know.

          During the review of MAPREDUCE-4049 we found inconsistencies in the naming and we aligned them with the other sub-tasks. We may need to do some more of that. This was your motivation to create MAPREDUCE-2454 branch after a similar comment I've made in MAPREDUCE-4809.

          You want to have gridmix runs in a reasonable size cluster to ensure there are not performance degradation due to the subtasks of MAPREDUCE-2454. I don' t see why MAPREDUCE-4049 should be excluded from those tests. Personally I think this is not needed for any of the patches as a change from 'new' to 'ReflectionUtils.newInstance()' outside of the processing loop cannot affect things, but you strongly asked me for this over the phone.

          Thus, I think your 'requirements' for the other tasks to MAPREDUCE-2454 do also apply to MAPREDUCE-4049 and until they are satisfied, MAPREDUCE-2454 is not ready for going to trunk.

          Said this, again, please revert. I'm confident we can do a last push and get the branch MAPREDUCE-2454 merge into trunk at fast pace.

          Show
          Alejandro Abdelnur added a comment - Arun, focusing on the technical side of your comments. My reasons to revert the patch from trunk are: All these components are highly interrelated as you know. During the review of MAPREDUCE-4049 we found inconsistencies in the naming and we aligned them with the other sub-tasks. We may need to do some more of that. This was your motivation to create MAPREDUCE-2454 branch after a similar comment I've made in MAPREDUCE-4809 . You want to have gridmix runs in a reasonable size cluster to ensure there are not performance degradation due to the subtasks of MAPREDUCE-2454 . I don' t see why MAPREDUCE-4049 should be excluded from those tests. Personally I think this is not needed for any of the patches as a change from 'new' to 'ReflectionUtils.newInstance()' outside of the processing loop cannot affect things, but you strongly asked me for this over the phone. Thus, I think your 'requirements' for the other tasks to MAPREDUCE-2454 do also apply to MAPREDUCE-4049 and until they are satisfied, MAPREDUCE-2454 is not ready for going to trunk. Said this, again, please revert. I'm confident we can do a last push and get the branch MAPREDUCE-2454 merge into trunk at fast pace.
          Hide
          Arun C Murthy added a comment -

          I'm getting tired of the lawyering.

          MAPREDUCE-2454 is a couple of orders of magnitude larger than MAPREDUCE-4049.

          Anyway to stop wasting my time arguing, I just started a 300-node gridmix run with MAPREDUCE-4049, I'll report back by eod.

          Show
          Arun C Murthy added a comment - I'm getting tired of the lawyering. MAPREDUCE-2454 is a couple of orders of magnitude larger than MAPREDUCE-4049 . Anyway to stop wasting my time arguing, I just started a 300-node gridmix run with MAPREDUCE-4049 , I'll report back by eod.
          Hide
          Alejandro Abdelnur added a comment -

          Sounds good on the 300 node gridmix test, mind running a full MR-2454 patch if Asokan posts a updated patch soon?

          Still this is not addressing:

          During the review of MAPREDUCE-4049 we found inconsistencies in the naming and we aligned them with the other sub-tasks. We may need to do some more of that. This was your motivation to create MAPREDUCE-2454 branch after a similar comment I've made in MAPREDUCE-4809.

          And this was significant enough for you to create MR-2454 branch.

          So again, revert MAPREDUCE-4049 from trunk until we iron out the whole branch.

          PS: the common ratio for order of magnitude is 10. I think you are a bit off with your comment. Lets keep things objective.

          Show
          Alejandro Abdelnur added a comment - Sounds good on the 300 node gridmix test, mind running a full MR-2454 patch if Asokan posts a updated patch soon? Still this is not addressing: During the review of MAPREDUCE-4049 we found inconsistencies in the naming and we aligned them with the other sub-tasks. We may need to do some more of that. This was your motivation to create MAPREDUCE-2454 branch after a similar comment I've made in MAPREDUCE-4809 . And this was significant enough for you to create MR-2454 branch. So again, revert MAPREDUCE-4049 from trunk until we iron out the whole branch. PS: the common ratio for order of magnitude is 10. I think you are a bit off with your comment. Lets keep things objective.
          Hide
          Alejandro Abdelnur added a comment -

          During the review of MAPREDUCE-4049 we found inconsistencies in the naming and we aligned them with the other sub-tasks. We may need to do some more of that. This was your motivation to create MAPREDUCE-2454 branch after a similar comment I've made in MAPREDUCE-4809.

          Arun, until we address this, are you reverting the patch? Or I'll have to do it?

          Show
          Alejandro Abdelnur added a comment - During the review of MAPREDUCE-4049 we found inconsistencies in the naming and we aligned them with the other sub-tasks. We may need to do some more of that. This was your motivation to create MAPREDUCE-2454 branch after a similar comment I've made in MAPREDUCE-4809 . Arun, until we address this, are you reverting the patch? Or I'll have to do it?
          Hide
          Arun C Murthy added a comment -

          Alejandro - you committed the patch. I'm confused. We can change them via MAPREDUCE-2454 if you are pedantic, or file a follow on issue.

          I've already started the runs, I'm not going to be able to get cluster time to re-run them.

          Show
          Arun C Murthy added a comment - Alejandro - you committed the patch. I'm confused. We can change them via MAPREDUCE-2454 if you are pedantic, or file a follow on issue. I've already started the runs, I'm not going to be able to get cluster time to re-run them.
          Hide
          Arun C Murthy added a comment -

          I'm done discussing. Can we please move on? File a follow on issue for naming nits.

          Show
          Arun C Murthy added a comment - I'm done discussing. Can we please move on? File a follow on issue for naming nits.
          Hide
          Alejandro Abdelnur added a comment -

          My +1 was for the work in the branch.

          My rationale it only echoing yours:

          https://issues.apache.org/jira/browse/MAPREDUCE-4809?focusedCommentId=13501245&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-13501245

          > Arun C Murthy added a comment - 20/Nov/12 07:50
          > I've also created a MR-2454 branch in svn, let's commit to that branch first.
          > This way we can change our mind before we do the final merge if necessary.

          MAPREDUCE-4812 will require some changes in MAPREDUCE-4049

          So, revert from trunk, thx.

          Show
          Alejandro Abdelnur added a comment - My +1 was for the work in the branch. My rationale it only echoing yours: https://issues.apache.org/jira/browse/MAPREDUCE-4809?focusedCommentId=13501245&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-13501245 > Arun C Murthy added a comment - 20/Nov/12 07:50 > I've also created a MR-2454 branch in svn, let's commit to that branch first. > This way we can change our mind before we do the final merge if necessary. MAPREDUCE-4812 will require some changes in MAPREDUCE-4049 So, revert from trunk, thx.
          Hide
          Alejandro Abdelnur added a comment -

          Arun,

          I'd appreciate not to remove again this JIRA from being a subtask of MAPREDUCE-2454.

          You are leaving with no choice but to -1 this patch to go in trunk until the work in the branch is completed.

          My rationale for this -1 it is yours:

          https://issues.apache.org/jira/browse/MAPREDUCE-4809?focusedCommentId=13501245&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-13501245

          > Arun C Murthy added a comment - 20/Nov/12 07:50
          > I've also created a MR-2454 branch in svn, let's commit to that branch first.
          > This way we can change our mind before we do the final merge if necessary.

          In addition, as we may need to make further changes to MAPREDUCE-4049, I'd like to see the whole scope of changes before merging back to trunk.

          Show
          Alejandro Abdelnur added a comment - Arun, I'd appreciate not to remove again this JIRA from being a subtask of MAPREDUCE-2454 . You are leaving with no choice but to -1 this patch to go in trunk until the work in the branch is completed. My rationale for this -1 it is yours: https://issues.apache.org/jira/browse/MAPREDUCE-4809?focusedCommentId=13501245&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-13501245 > Arun C Murthy added a comment - 20/Nov/12 07:50 > I've also created a MR-2454 branch in svn, let's commit to that branch first. > This way we can change our mind before we do the final merge if necessary. In addition, as we may need to make further changes to MAPREDUCE-4049 , I'd like to see the whole scope of changes before merging back to trunk.
          Hide
          Avner BenHanoch added a comment -

          Alejandro,

          I am repeating my previous comment that your behavior is inappropriate and unfriendly.

          • Your interest in this JIRA issue was only started to be shown 2 weeks ago, exactly 2 hours after Arun's last comment in MAPREDUCE-2454, in which Arun kept persisting his concern for the core of MapReduce with the massive changes that the huge patch there introduces, and requested breaking it into subtasks.
          • At that phase my trivial patch already passed a long way and was ready for the trunk (beside minor last request just to rename functions in one class). But then you woke up and started to ask many new things.

          I was always concerned that linking our issues will delay one for the other, instead of letting each progress at its own pace (and this is just 3 months after Asokan already tried to delay my issue and wanted to first submit his patch).

          You promised many times that all this will delay me just in few days. See the course of your promises to me (all quotes are taken from you in this JIRA):

          • Your original justification for the linking was: "As all this JIRAs are small, I think we'll be able to move fast with all of them".
          • You responded: "If that requires a couple of extra days, is is a small price to pay".
          • You clarified: "And don't worry about begin a subtask delaying it, I'll review it as soon as you post a patch and committed it when ready. The same is happening with the other subtasks, so things should be in quite quickly. Thx"

          Now, what happened:

          • I fulfilled ALL your requests including those that are "to have a consistent set of names and APIs (ie inner Context) for a set of related plugins (all the ones affected by MAPREDUCE-2454)", then, you personally reviewed my patch and you personally +1 it.
          • This Friday you promised again to merge to trunk -"fast, by the end of next week if no surprises arise".
          • Then you personally merged it to your branch. After that Arun merged it to trunk too.
          • Then you broke all your past commitments and responded with: "-1 this patch to go in trunk until the work in the branch is completed."

          Sorry. I don’t get it!

          • My patch contains all your requests, including those that are "to have a consistent set of names and APIs (ie inner Context) for a set of related plugins (all the ones affected by MAPREDUCE-2454)", and you personally +1 it and merged it to your branch. How can be that it suits your branch, but not the trunk because of your branch needs???
          • Personally, watching the design (and performance) questions on MAPREDUCE-2454 I have no idea when this patch will ever be accepted to trunk. I don’t think it is appropriate to block my trivial patch to go to the trunk. My patch stands for itself. There are many people that are waiting for it and wanting it.
          • Per your request, my patch got the tags @Unstable and @limittedPrivate. You always have the option to iron its code, in the same way that you can do with any code in SVN.
          • Your "-1 this patch to go in trunk until the work in the branch is completed" literally says that you took my issue as hostage for MAPREDUCE-2454 despite your promises that these steps will not delay me.
          Show
          Avner BenHanoch added a comment - Alejandro, I am repeating my previous comment that your behavior is inappropriate and unfriendly. Your interest in this JIRA issue was only started to be shown 2 weeks ago, exactly 2 hours after Arun's last comment in MAPREDUCE-2454 , in which Arun kept persisting his concern for the core of MapReduce with the massive changes that the huge patch there introduces, and requested breaking it into subtasks. At that phase my trivial patch already passed a long way and was ready for the trunk (beside minor last request just to rename functions in one class). But then you woke up and started to ask many new things. I was always concerned that linking our issues will delay one for the other, instead of letting each progress at its own pace (and this is just 3 months after Asokan already tried to delay my issue and wanted to first submit his patch). You promised many times that all this will delay me just in few days. See the course of your promises to me (all quotes are taken from you in this JIRA) : Your original justification for the linking was: "As all this JIRAs are small, I think we'll be able to move fast with all of them " . You responded: "If that requires a couple of extra days , is is a small price to pay" . You clarified: "And don't worry about begin a subtask delaying it, I'll review it as soon as you post a patch and committed it when ready. The same is happening with the other subtasks, so things should be in quite quickly . Thx" Now, what happened: I fulfilled ALL your requests including those that are "to have a consistent set of names and APIs (ie inner Context) for a set of related plugins (all the ones affected by MAPREDUCE-2454 )" , then, you personally reviewed my patch and you personally +1 it . This Friday you promised again to merge to trunk - "fast, by the end of next week if no surprises arise" . Then you personally merged it to your branch . After that Arun merged it to trunk too . Then you broke all your past commitments and responded with: "-1 this patch to go in trunk until the work in the branch is completed." Sorry. I don’t get it! My patch contains all your requests, including those that are "to have a consistent set of names and APIs (ie inner Context) for a set of related plugins (all the ones affected by MAPREDUCE-2454 )" , and you personally +1 it and merged it to your branch. How can be that it suits your branch, but not the trunk because of your branch needs??? Personally, watching the design (and performance) questions on MAPREDUCE-2454 I have no idea when this patch will ever be accepted to trunk. I don’t think it is appropriate to block my trivial patch to go to the trunk. My patch stands for itself. There are many people that are waiting for it and wanting it. Per your request, my patch got the tags @Unstable and @limittedPrivate. You always have the option to iron its code, in the same way that you can do with any code in SVN. Your "-1 this patch to go in trunk until the work in the branch is completed" literally says that you took my issue as hostage for MAPREDUCE-2454 despite your promises that these steps will not delay me.
          Hide
          Avner BenHanoch added a comment -

          If you need more:

          • My issue is a PART OF whole new topic of Shuffle Consumer – Shuffle Provider plugins. Currently, we just submitted the consumer part. We still need to complete the provider part in MRv2 and in MRv1, plus few related topics. Then we need to back port all to hadoop-2 & hadoop-1.
          • Hence, my issue is part of other big context and not part of your issue (Still, be my guest, and feel free to subordinate your issue to my issue)
          • Besides, it was already clearly said that at any case, MAPREDUCE-2454 can’t be accepted to hadoop-1, since it is too massive change for a branch that is going to its end of life. On the other hand, my patch already passed code review for hadoop-1 and was only delayed because of a justified request to go in the regular path and first submit to trunk. Hence, there is no reason to block my trivial patch for all branches just because the complex issues in MAPREDUCE-2454.
          Show
          Avner BenHanoch added a comment - If you need more: My issue is a PART OF whole new topic of Shuffle Consumer – Shuffle Provider plugins. Currently, we just submitted the consumer part. We still need to complete the provider part in MRv2 and in MRv1, plus few related topics. Then we need to back port all to hadoop-2 & hadoop-1. Hence, my issue is part of other big context and not part of your issue (Still, be my guest, and feel free to subordinate your issue to my issue) Besides, it was already clearly said that at any case, MAPREDUCE-2454 can’t be accepted to hadoop-1, since it is too massive change for a branch that is going to its end of life. On the other hand, my patch already passed code review for hadoop-1 and was only delayed because of a justified request to go in the regular path and first submit to trunk. Hence, there is no reason to block my trivial patch for all branches just because the complex issues in MAPREDUCE-2454 .
          Hide
          Alejandro Abdelnur added a comment -

          Avner,

          If you look at the latest patches for MAPREDUCE-4807, MAPREDUCE-4812 & MAPREDUCE-4809, you'll see that they are limited to do the same thing MAPREDUCE-4049 does, define an interface, make existing classes to implement that inteface, instanciate those classes using ReflectionUtils.newInstance(). The only thing extra is a minor refactoring that it has been already agreed that it is OK and posses no risk. The bulk of the patches are testcases, that instead limiting to test the pluggability, they provide alternate simple alternate implementations to show the interfaces are adequate for such.

          In order to get this done, I encourage you, as a contributor, to look at the work proposed in MAPREDUCE-4807, MAPREDUCE-4812 & MAPREDUCE-4809 and provide feedback so we get things in the branch and in trunk.

          Show
          Alejandro Abdelnur added a comment - Avner, If you look at the latest patches for MAPREDUCE-4807 , MAPREDUCE-4812 & MAPREDUCE-4809 , you'll see that they are limited to do the same thing MAPREDUCE-4049 does, define an interface, make existing classes to implement that inteface, instanciate those classes using ReflectionUtils.newInstance(). The only thing extra is a minor refactoring that it has been already agreed that it is OK and posses no risk. The bulk of the patches are testcases, that instead limiting to test the pluggability, they provide alternate simple alternate implementations to show the interfaces are adequate for such. In order to get this done, I encourage you, as a contributor, to look at the work proposed in MAPREDUCE-4807 , MAPREDUCE-4812 & MAPREDUCE-4809 and provide feedback so we get things in the branch and in trunk.
          Hide
          Arun C Murthy added a comment -

          Forgot to add over weekend - GM run finished with this patch slightly faster 13s on 300 nodes with ~1300 jobs. Overall runtime was 65mins.

          Show
          Arun C Murthy added a comment - Forgot to add over weekend - GM run finished with this patch slightly faster 13s on 300 nodes with ~1300 jobs. Overall runtime was 65mins.
          Hide
          Milind Bhandarkar added a comment -

          Thanks for verifying, Arun. FWIW, we have been running with many earlier versions of this patch on our Greenplum Analytics Workbench 1000 node cluster since May 2012 (I think I had mentioned this to you and Chris Douglas during Hadoop Summit in June), and haven't found any issues with this patch so far. (See my comment above.)

          Show
          Milind Bhandarkar added a comment - Thanks for verifying, Arun. FWIW, we have been running with many earlier versions of this patch on our Greenplum Analytics Workbench 1000 node cluster since May 2012 (I think I had mentioned this to you and Chris Douglas during Hadoop Summit in June), and haven't found any issues with this patch so far. (See my comment above.)
          Hide
          Chris Douglas added a comment -

          This is clearly related to MAPREDUCE-2454. Many of the final patches are even in similar styles after thorough review. But the 2454 branch was supposed to make it easier to review Mariappan Asokan's extensive refactoring, not cover every API change to MapReduce supporting extensions. These interfaces aren't permanent, there will be more issues proposing hooks and callbacks after these, and the more expansive patches will get revised in different branches. It's routine; everyone please chill.

          I read the rest of the MAPREDUCE-2454 subtasks and reviewed the outstanding and committed patches. It looks like a good first pass for adding hooks to Tasks, ready to be committed and merged to trunk presently. Alejandro: OK with you if we close this as resolved?

          Show
          Chris Douglas added a comment - This is clearly related to MAPREDUCE-2454 . Many of the final patches are even in similar styles after thorough review. But the 2454 branch was supposed to make it easier to review Mariappan Asokan 's extensive refactoring, not cover every API change to MapReduce supporting extensions. These interfaces aren't permanent, there will be more issues proposing hooks and callbacks after these, and the more expansive patches will get revised in different branches. It's routine; everyone please chill. I read the rest of the MAPREDUCE-2454 subtasks and reviewed the outstanding and committed patches. It looks like a good first pass for adding hooks to Tasks, ready to be committed and merged to trunk presently. Alejandro: OK with you if we close this as resolved?
          Hide
          Laxman added a comment -

          With this patch, we are able to meet our goals (plugin custom shuffle algorithm "Network Levitate Merge") without any issues. Thanks Avner for keeping the patch small and crisp.

          Show
          Laxman added a comment - With this patch, we are able to meet our goals (plugin custom shuffle algorithm "Network Levitate Merge") without any issues. Thanks Avner for keeping the patch small and crisp.
          Hide
          Alejandro Abdelnur added a comment -

          Chris thanks for taking a look at these JIRAs.

          Withdrawing my -1. I'm OK with doing necessary tweaks in follow up JIRAs.

          I'd suggest we merge MAPREDUCE-4809 and MAPREDUCE-4807 to trunk. Then we can do the incremental work for MAPREDUCE-4812 and MAPREDUCE-4808 in trunk as well.

          Arun, is that OK with you?

          Show
          Alejandro Abdelnur added a comment - Chris thanks for taking a look at these JIRAs. Withdrawing my -1. I'm OK with doing necessary tweaks in follow up JIRAs. I'd suggest we merge MAPREDUCE-4809 and MAPREDUCE-4807 to trunk. Then we can do the incremental work for MAPREDUCE-4812 and MAPREDUCE-4808 in trunk as well. Arun, is that OK with you?
          Hide
          Alejandro Abdelnur added a comment -

          Avner, it seems the attachment you posted on Monday for branch-1 is MIA, would you please post it again? thx.

          Show
          Alejandro Abdelnur added a comment - Avner, it seems the attachment you posted on Monday for branch-1 is MIA, would you please post it again? thx.
          Hide
          Avner BenHanoch added a comment -

          Hi Alejandro,
          On Monday I only removed obsolete attachments for the trunk and kept just the last one we submitted.

          Speaking about that, please let me know:
          1. Do you prefer patch for branch-1 in this issue or in a separated issue.
          2. There is still what to do in the trunk for ShuffleProvider - see this comment. Do you want me to address it here or in a separated issue.

          Kindly thank you,
          Avner

          Show
          Avner BenHanoch added a comment - Hi Alejandro, On Monday I only removed obsolete attachments for the trunk and kept just the last one we submitted. Speaking about that, please let me know: 1. Do you prefer patch for branch-1 in this issue or in a separated issue. 2. There is still what to do in the trunk for ShuffleProvider - see this comment . Do you want me to address it here or in a separated issue. Kindly thank you, Avner
          Hide
          Alejandro Abdelnur added a comment -

          Avner, thanks for the clarification, I've got confused as JIRA emailed an 'updated patch' message.

          On #1, the ShuffleConsumerPlugin should be in this JIRA.
          On #2, I assume the trunk version does not (will not) have a Map side because the ShuffleHandler is already pluggable. Given that, the Map side (ShuffleProvider) seems an artifact of the backport of this JIRA. Because of that, I think is OK to have it here.

          I assume you are working on updating the attached Hadoop-1 patch, following some comments on the current Hadoop-1 patch:

          • Not having a ShuffleProviderPlugin in the TaskTracker should be reason to fail the TaskTracker at startup, no?
          • We should follow the same pattern as in trunk:
            • Define an interface instead of an abstract class for ShuffleConsumerPlugin, with init(), fetchOutput(), createKVIterator(), getMergeThrowable() methods.
            • Define a Context for ShuffleConsumerPlugin initialization
            • Use ReflectionUtil.newInstance() in ReducerTask to instantiate the ShuffleConsumerPlugin
            • visibility/stability Annotations are missing
          Show
          Alejandro Abdelnur added a comment - Avner, thanks for the clarification, I've got confused as JIRA emailed an 'updated patch' message. On #1, the ShuffleConsumerPlugin should be in this JIRA. On #2, I assume the trunk version does not (will not) have a Map side because the ShuffleHandler is already pluggable. Given that, the Map side (ShuffleProvider) seems an artifact of the backport of this JIRA. Because of that, I think is OK to have it here. I assume you are working on updating the attached Hadoop-1 patch, following some comments on the current Hadoop-1 patch: Not having a ShuffleProviderPlugin in the TaskTracker should be reason to fail the TaskTracker at startup, no? We should follow the same pattern as in trunk: Define an interface instead of an abstract class for ShuffleConsumerPlugin, with init(), fetchOutput(), createKVIterator(), getMergeThrowable() methods. Define a Context for ShuffleConsumerPlugin initialization Use ReflectionUtil.newInstance() in ReducerTask to instantiate the ShuffleConsumerPlugin visibility/stability Annotations are missing
          Hide
          Avner BenHanoch added a comment -

          Hi Alejandro,
          thanks for your comments!

          my 2nd point above was for the trunk. True, in MRv2 AuxiliaryServices are plugable, still they don't get APPLICATION_INIT events; hence, they don't know to map jobId->userId. We need to send this event to all AuxiliaryServices, or to define 2 groups of AuxiliaryServices: 1 that get this event, and 1 that doesn't get this event. In the current code, this event is only sent to "mapreduce.shuffle" using hard-coded string rather than relying on any conf settings.

          For the branch-1 comments:
          Please notice that ShuffleProvider has different semantics than ShuffleConsumer, because a 3rd party shuffle-provider always runs in addition to the default shuffle-provider. multiple jobs can run in parallel, resulting in various shuffleConsumers in parallel (in different Jobs/ReduceTasks). Hence, all possible providers should exists in parallel. Saying that, the semantic of ShuffleProvider plugin is in addition to the default shuffle-provider. Hence TT should not fail for that.

          (for the rest of your branch-1 comments: yes, you are right on all!)

          Show
          Avner BenHanoch added a comment - Hi Alejandro, thanks for your comments! my 2nd point above was for the trunk. True, in MRv2 AuxiliaryServices are plugable, still they don't get APPLICATION_INIT events; hence, they don't know to map jobId->userId. We need to send this event to all AuxiliaryServices, or to define 2 groups of AuxiliaryServices: 1 that get this event, and 1 that doesn't get this event. In the current code, this event is only sent to "mapreduce.shuffle" using hard-coded string rather than relying on any conf settings . For the branch-1 comments: Please notice that ShuffleProvider has different semantics than ShuffleConsumer, because a 3rd party shuffle-provider always runs in addition to the default shuffle-provider. multiple jobs can run in parallel, resulting in various shuffleConsumers in parallel (in different Jobs/ReduceTasks). Hence, all possible providers should exists in parallel. Saying that, the semantic of ShuffleProvider plugin is in addition to the default shuffle-provider. Hence TT should not fail for that. (for the rest of your branch-1 comments: yes, you are right on all!)
          Hide
          Alejandro Abdelnur added a comment -

          my 2nd point above was for the trunk.

          If that is the case, I think we should do that in a follow up JIRA and have there the patch for trunk and branch-1.

          because a 3rd party shuffle-provider always runs in addition to the default shuffle-provider.

          The shuffle-provider class is a TaskTracker config, so it is the same for ALL jobs; meaning the TaskTracker will use always the same shuffle-provider class. no?

          Show
          Alejandro Abdelnur added a comment - my 2nd point above was for the trunk. If that is the case, I think we should do that in a follow up JIRA and have there the patch for trunk and branch-1. because a 3rd party shuffle-provider always runs in addition to the default shuffle-provider. The shuffle-provider class is a TaskTracker config, so it is the same for ALL jobs; meaning the TaskTracker will use always the same shuffle-provider class. no?
          Hide
          Avner BenHanoch added a comment -

          1. so I'll open case for the trunk "send APPLICATION_INIT event to additional AuxiliaryServices instead of hard-coded send to 'mapreduce.shuffle'"

          (1.a Do you have an idea whether to send it to all of them, or to use two sets of AuxiliaryServices - 1 that get the event and 1 that doesn't get it?)

          2. My branch-1 code only loads an optionally configured ShuffleProviderPlugin. I didn't touch the existing code that loads MapOutputServlet in the CTOR of TT. Hence, user will have 1 or 2 shuffle-providers.

          Show
          Avner BenHanoch added a comment - 1. so I'll open case for the trunk "send APPLICATION_INIT event to additional AuxiliaryServices instead of hard-coded send to 'mapreduce.shuffle'" (1.a Do you have an idea whether to send it to all of them, or to use two sets of AuxiliaryServices - 1 that get the event and 1 that doesn't get it?) 2. My branch-1 code only loads an optionally configured ShuffleProviderPlugin. I didn't touch the existing code that loads MapOutputServlet in the CTOR of TT. Hence, user will have 1 or 2 shuffle-providers.
          Hide
          Alejandro Abdelnur added a comment -

          On #1, IMO APPLICATION_INIT should be sent to all auxiliary services (and APPLICATION_STOP)

          On #2, is there a use case to load both instead just the configured one?

          Show
          Alejandro Abdelnur added a comment - On #1, IMO APPLICATION_INIT should be sent to all auxiliary services (and APPLICATION_STOP) On #2, is there a use case to load both instead just the configured one?
          Hide
          Avner BenHanoch added a comment -

          Hi Alejandro,

          On #1 - Thanks!

          On #2 - YES:
          1. Since, ShuffleProvider is configured for the lifetime of TT; while, ShuffleConsumer is configured per job. We don't want to restart MapReduce/TaskTrackers any time we want to use different shuffle.

          2. In addition, I expect that for 1 job there will be used just 1 type of shuffle. Still, TT supports multiple jobs of multiple users with different shuffle&merge needs in parallel. Hence, multiple shuffle consumers may run in parallel (in the multiple jobs) => they will request data from multiple providers. => TT needs multiple providers in parallel (You can consider multiple ShufleProviders in MRv1 as equivalent to multiple AuxiliaryServices that are allowed in MRv2).

          3. It could be that a ShuffleConsumerX will be ideal for jobs of one type, while ShuffleConsumerY will be ideal for jobs of other type (for example Grep vs. TeraSort). Hence, multiple Shuffle-Consumer plugins may run in parallel in multiple jobs. Each of the consumers will contact its desired shuffle provider. Hence, all providers should be available in parallel (also, one shuffle service can be sensitive to type of network problems that doesn't disturb other shuffle services, hence, it should be possible to fallback to another shuffle on the fly).

          on the design:
          1. It is clear that a ShuffleProvider is a daemon like TT, while ShuffleConsumer is a client that lives in the context of RT
          2. It is clear that multiple providers can run in parallel and each is able to serve shuffle request it gets.
          3. A shuffle consumer instance will only contact one of the shuffle providers and will request its desired files only from from this provider.
          4. multiple consumers in multiple jobs may contact different providers
          5. A shuffle provider that doesn't serve a request doesn't consume resources for it.

          regards,
          Avner

          Show
          Avner BenHanoch added a comment - Hi Alejandro, On #1 - Thanks! On #2 - YES: 1. Since, ShuffleProvider is configured for the lifetime of TT; while, ShuffleConsumer is configured per job. We don't want to restart MapReduce/TaskTrackers any time we want to use different shuffle. 2. In addition, I expect that for 1 job there will be used just 1 type of shuffle. Still, TT supports multiple jobs of multiple users with different shuffle&merge needs in parallel . Hence, multiple shuffle consumers may run in parallel (in the multiple jobs) => they will request data from multiple providers. => TT needs multiple providers in parallel (You can consider multiple ShufleProviders in MRv1 as equivalent to multiple AuxiliaryServices that are allowed in MRv2). 3. It could be that a ShuffleConsumerX will be ideal for jobs of one type, while ShuffleConsumerY will be ideal for jobs of other type (for example Grep vs. TeraSort). Hence, multiple Shuffle-Consumer plugins may run in parallel in multiple jobs. Each of the consumers will contact its desired shuffle provider. Hence, all providers should be available in parallel (also, one shuffle service can be sensitive to type of network problems that doesn't disturb other shuffle services, hence, it should be possible to fallback to another shuffle on the fly). on the design: 1. It is clear that a ShuffleProvider is a daemon like TT, while ShuffleConsumer is a client that lives in the context of RT 2. It is clear that multiple providers can run in parallel and each is able to serve shuffle request it gets. 3. A shuffle consumer instance will only contact one of the shuffle providers and will request its desired files only from from this provider. 4. multiple consumers in multiple jobs may contact different providers 5. A shuffle provider that doesn't serve a request doesn't consume resources for it. regards, Avner
          Hide
          Alejandro Abdelnur added a comment -

          Got it, thxs for the detailed explanation.

          • Does this mean that providers must lazy initialize on the first request?
          • Are you planing to support loading N providers in your patch?
          Show
          Alejandro Abdelnur added a comment - Got it, thxs for the detailed explanation. Does this mean that providers must lazy initialize on the first request? Are you planing to support loading N providers in your patch?
          Hide
          Avner BenHanoch added a comment -

          1. I don't use the term "must". Each provider can choose its desired optimization. If the performance of a provider worth for the user the user will use it. In general, I think that the major resouce that is used by providers is the cache of MOFs. Since this cache is filled upon serving requests than the price of unused provider that was loaded is cheap. Other than that, I think that providers mainly listen for incoming requests.

          2. In my patch I plan to support just 1 provider (in addition to the built in MapOutputHttpServlet). This is enough for my use case. Support of N providers is legitimate idea. If it is needed by someone, I prefer that it will be handled outside my patch.

          Show
          Avner BenHanoch added a comment - 1. I don't use the term "must". Each provider can choose its desired optimization. If the performance of a provider worth for the user the user will use it. In general, I think that the major resouce that is used by providers is the cache of MOFs. Since this cache is filled upon serving requests than the price of unused provider that was loaded is cheap. Other than that, I think that providers mainly listen for incoming requests. 2. In my patch I plan to support just 1 provider (in addition to the built in MapOutputHttpServlet). This is enough for my use case. Support of N providers is legitimate idea. If it is needed by someone, I prefer that it will be handled outside my patch.
          Hide
          Avner BenHanoch added a comment -

          Hi Alejandro,

          re #2, my intuation is that supporting 1 external shuffle service (in addition to the built-in shuffle service) is the "keep it simple" solution. I feel that the use case of N providers is theoretical. Hence, I prefer to keep the conf and code simple.

          Avner

          Show
          Avner BenHanoch added a comment - Hi Alejandro, re #2, my intuation is that supporting 1 external shuffle service (in addition to the built-in shuffle service) is the "keep it simple" solution. I feel that the use case of N providers is theoretical. Hence, I prefer to keep the conf and code simple. Avner
          Hide
          Avner BenHanoch added a comment -

          Hi Alejandro & Arun,

          I just attached a patch for branch-1 (that also suits branch-1.1).
          This patch is just port of the original patch from trunk into branch-1 and contains all accumulated comments.

          I am not indicating "patch available" as this will trigger false AutoQA with trunk instead of branch-1.
          I welcome your review and commit.
          regards,
          Avner

          Show
          Avner BenHanoch added a comment - Hi Alejandro & Arun, I just attached a patch for branch-1 (that also suits branch-1.1). This patch is just port of the original patch from trunk into branch-1 and contains all accumulated comments. I am not indicating "patch available" as this will trigger false AutoQA with trunk instead of branch-1. I welcome your review and commit. regards, Avner
          Hide
          Alejandro Abdelnur added a comment -

          Avner, just done a quick look and seems good, will do a full review later today. I think we should do the ShuffleProviderPlugin changes in a different JIRA, thus this one would be just a backport of the ShuffleConsumerProvider.

          Show
          Alejandro Abdelnur added a comment - Avner, just done a quick look and seems good, will do a full review later today. I think we should do the ShuffleProviderPlugin changes in a different JIRA, thus this one would be just a backport of the ShuffleConsumerProvider.
          Hide
          Avner BenHanoch added a comment -

          Hi Alejandro,

          Thanks for your quick handling - I appereciate that!

          This issue is devoted to "generic shuffle service", including both provider & consumer part (even properties' names in xml have same scheme). Arun reviewed the branch-1 patch at July 13, 2012 and left with just minor nits that were all handled long time ago.
          Following your guideline: "I assume the trunk version does not (will not) have a Map side because the ShuffleHandler is already pluggable. Given that, the Map side (ShuffleProvider) seems an artifact of the backport of this JIRA. Because of that, I think is OK to have it here."

          Saying all the above, I'll be happy to have this subject concentrated in one place and in one piece.

          Avner

          Show
          Avner BenHanoch added a comment - Hi Alejandro, Thanks for your quick handling - I appereciate that! This issue is devoted to "generic shuffle service", including both provider & consumer part (even properties' names in xml have same scheme). Arun reviewed the branch-1 patch at July 13, 2012 and left with just minor nits that were all handled long time ago. Following your guideline: "I assume the trunk version does not (will not) have a Map side because the ShuffleHandler is already pluggable. Given that, the Map side (ShuffleProvider) seems an artifact of the backport of this JIRA. Because of that, I think is OK to have it here." Saying all the above, I'll be happy to have this subject concentrated in one place and in one piece. Avner
          Hide
          Alejandro Abdelnur added a comment -

          Full review now.

          • ReduceTask.java: RT_SHUFFLE_CONSUMERER_PLUGIN constant name has a typo 'CONSUMERER'
          • ReduceTask.java: RT_SHUFFLE_CONSUMERER_PLUGIN constant should be defined in JobContext.java
          • ReducerTask.java: instantiation of the ShuffleConsumerPlugin should alwasy be done viea ReflectionUtils.newInstance() regardless if it is the default or not.
          • ReducerCopier class should be made public static in order to be able to be created via ReflectionUtils.newInstance()
          • TaskTracker.java: the value of the constant TT_SHUFFLE_PROVIDER_PLUGIN should not have 'job' as this plugin is not per job but per TT.
          • TestShufflePlugin.java: it has several unused imports
          • TestShufflePlugin.java: testConsumer() method does not use the dirs var, if not need it should be removed.

          I'm not trilled about the TT loading the default shuffle provider (which is not implementting the new shuffle provider interface) and in addition one extra custom shuffle provider.

          Instead, I'd say the current shuffle provider logic should be refactored into a shuffle provider implementation and this one loaded by default. And, if as you indicated before, you want to load different impls simultaneously, then a shuffle plugin multiplexor implementation could be used.

          This increases the scope of the changes, thus why I'd like to do this in a separate JIRA and keep this JIRA for the consumer (reducer) side.

          Show
          Alejandro Abdelnur added a comment - Full review now. ReduceTask.java: RT_SHUFFLE_CONSUMERER_PLUGIN constant name has a typo 'CONSUMERER' ReduceTask.java: RT_SHUFFLE_CONSUMERER_PLUGIN constant should be defined in JobContext.java ReducerTask.java: instantiation of the ShuffleConsumerPlugin should alwasy be done viea ReflectionUtils.newInstance() regardless if it is the default or not. ReducerCopier class should be made public static in order to be able to be created via ReflectionUtils.newInstance() TaskTracker.java: the value of the constant TT_SHUFFLE_PROVIDER_PLUGIN should not have 'job' as this plugin is not per job but per TT. TestShufflePlugin.java: it has several unused imports TestShufflePlugin.java: testConsumer() method does not use the dirs var, if not need it should be removed. I'm not trilled about the TT loading the default shuffle provider (which is not implementting the new shuffle provider interface) and in addition one extra custom shuffle provider. Instead, I'd say the current shuffle provider logic should be refactored into a shuffle provider implementation and this one loaded by default. And, if as you indicated before, you want to load different impls simultaneously, then a shuffle plugin multiplexor implementation could be used. This increases the scope of the changes, thus why I'd like to do this in a separate JIRA and keep this JIRA for the consumer (reducer) side.
          Hide
          Alejandro Abdelnur added a comment -

          Forgot one thing in my prev comment. This is a question for committers, are allowing the addition of new features to Hadoop 1? I'm OK with it, but we should have a clear stance on it.

          Show
          Alejandro Abdelnur added a comment - Forgot one thing in my prev comment. This is a question for committers, are allowing the addition of new features to Hadoop 1? I'm OK with it, but we should have a clear stance on it.
          Hide
          Alejandro Abdelnur added a comment -

          Additional review items:

          • I've just noticed, that your ShuffleConsumerPlugin API does not respect the API of the ReduceCopier, the createKVIterator() method has a different signature. The parameters being passed to it, in your patch, are already avail in the Context, except for the FileSystem, but you could create the FileSystem (and obtain the raw) within the your plugin impl using the conf received in the context.
          • the RT_SHUFFLE_CONSUMERER_PLUGIN constant, why the RT_? SHUFFLE_CONSUMER_PLUGIN_ATTR would follow the convention used by other constants.
          Show
          Alejandro Abdelnur added a comment - Additional review items: I've just noticed, that your ShuffleConsumerPlugin API does not respect the API of the ReduceCopier, the createKVIterator() method has a different signature. The parameters being passed to it, in your patch, are already avail in the Context, except for the FileSystem, but you could create the FileSystem (and obtain the raw) within the your plugin impl using the conf received in the context. the RT_SHUFFLE_CONSUMERER_PLUGIN constant, why the RT_? SHUFFLE_CONSUMER_PLUGIN_ATTR would follow the convention used by other constants.
          Hide
          Avner BenHanoch added a comment -

          Hi Alejandro - thanks for your thorough and fast review!

          regarding

          ReducerCopier class should be made public static in order to be able to be created via ReflectionUtils.newInstance()

          ... cool! Actually, I went in this direction in my very first patch. I am happy to return to it. (notice, that it will introduce changes in all places that currently ReduceCopier directly uses members of the encapsulating ReduceTask object - but, i believe this is correct thing)

          regarding:

          I've just noticed, that your ShuffleConsumerPlugin API does not respect the API of the ReduceCopier, the createKVIterator() method has a different signature. The parameters being passed to it, in your patch, are already avail in the Context, except for the FileSystem, but you could create the FileSystem (and obtain the raw) within the your plugin impl using the conf received in the context.

          I think this comment is wrong. Please clarify!

          Regarding

          I'm not trilled about the TT loading the default shuffle provider (which is not implementting the new shuffle provider interface) and in addition one extra custom shuffle provider.
          Instead, I'd say the current shuffle provider logic should be refactored into a shuffle provider implementation and this one loaded by default. And, if as you indicated before, you want to load different impls simultaneously, then a shuffle plugin multiplexor implementation could be used.
          This increases the scope of the changes, thus why I'd like to do this in a separate JIRA and keep this JIRA for the consumer (reducer) side.

          Actually, I wrote above "my intuition is that supporting 1 external shuffle service (in addition to the built-in shuffle service) is the 'keep it simple' solution. I feel that the use case of N providers is theoretical. Hence, I prefer to keep the conf and code simple". This clarify why I wrote my patch in this way instead of introducing big feature with "shuffle plugin multiplexor..." in hadoop-1.
          Again, this JIRA issue - since its creation - focus on "Support generic shuffle service as set of two plugins: ShuffleProvider & ShuffleConsumer". It has no value for me, if it deals with consumer only.

          I am fine with all the rest of your comments. Please let me know if I can continue according to this!
          Avner

          Show
          Avner BenHanoch added a comment - Hi Alejandro - thanks for your thorough and fast review! regarding ReducerCopier class should be made public static in order to be able to be created via ReflectionUtils.newInstance() ... cool! Actually, I went in this direction in my very first patch. I am happy to return to it. (notice, that it will introduce changes in all places that currently ReduceCopier directly uses members of the encapsulating ReduceTask object - but, i believe this is correct thing) regarding: I've just noticed, that your ShuffleConsumerPlugin API does not respect the API of the ReduceCopier, the createKVIterator() method has a different signature. The parameters being passed to it, in your patch, are already avail in the Context, except for the FileSystem, but you could create the FileSystem (and obtain the raw) within the your plugin impl using the conf received in the context. I think this comment is wrong. Please clarify! Regarding I'm not trilled about the TT loading the default shuffle provider (which is not implementting the new shuffle provider interface) and in addition one extra custom shuffle provider. Instead, I'd say the current shuffle provider logic should be refactored into a shuffle provider implementation and this one loaded by default. And, if as you indicated before, you want to load different impls simultaneously, then a shuffle plugin multiplexor implementation could be used. This increases the scope of the changes, thus why I'd like to do this in a separate JIRA and keep this JIRA for the consumer (reducer) side. Actually, I wrote above " my intuition is that supporting 1 external shuffle service (in addition to the built-in shuffle service) is the 'keep it simple' solution. I feel that the use case of N providers is theoretical. Hence, I prefer to keep the conf and code simple ". This clarify why I wrote my patch in this way instead of introducing big feature with "shuffle plugin multiplexor..." in hadoop-1. Again, this JIRA issue - since its creation - focus on " Support generic shuffle service as set of two plugins: ShuffleProvider & ShuffleConsumer ". It has no value for me, if it deals with consumer only. I am fine with all the rest of your comments. Please let me know if I can continue according to this! Avner
          Hide
          Alejandro Abdelnur added a comment -

          Regarding "...ShuffleConsumerPlugin API does not respect the API of the ReduceCopier,..." & "I think this comment is wrong. Please clarify!"... You are right, please disregard that comment. After integrating my comments into the consumer side I think it (the consumer) is ready to go in.

          Regarding the producer changes, I think that the default producer implementation should implement the producer plugin interface as well. Once we have that, the multiplexor plugin would be trivial, I'd be happy to help with that. We can do the producer plugin as a subtask of this JIRA.

          Show
          Alejandro Abdelnur added a comment - Regarding "...ShuffleConsumerPlugin API does not respect the API of the ReduceCopier,..." & "I think this comment is wrong. Please clarify!"... You are right, please disregard that comment. After integrating my comments into the consumer side I think it (the consumer) is ready to go in. Regarding the producer changes, I think that the default producer implementation should implement the producer plugin interface as well. Once we have that, the multiplexor plugin would be trivial, I'd be happy to help with that. We can do the producer plugin as a subtask of this JIRA.
          Hide
          Avner BenHanoch added a comment -

          Thanks. So, we agreed upon the consumer details.

          Now, For the producer details:

          • Again, throughout the lifetime of this JIRA issue, the consumer & producer come together, since they are the 2 sides of the shuffle service. This JIRA issue has no value if it has one without the other. Hence, they should be kept together!
            Additionally, I want it to be one patch that can be ported to any hadoop-1.x.y version at once.
          • The default producer implementation already implements the producer plugin interface! (though, it is still loaded via HttpServlet interface) As I said, I went on a "keep it simple" solution, in which I only support 1 extra provider (with simple code and simple conf). Please clarify whether this is enough, or rather you ask me to support N providers. I don't want to write a "new feature" and then have someone say that we have a problem to introduce "new feature" in hadoop-1.
          Show
          Avner BenHanoch added a comment - Thanks. So, we agreed upon the consumer details. Now, For the producer details: Again, throughout the lifetime of this JIRA issue, the consumer & producer come together, since they are the 2 sides of the shuffle service. This JIRA issue has no value if it has one without the other. Hence, they should be kept together! Additionally, I want it to be one patch that can be ported to any hadoop-1.x.y version at once. The default producer implementation already implements the producer plugin interface! (though, it is still loaded via HttpServlet interface) As I said, I went on a "keep it simple" solution, in which I only support 1 extra provider (with simple code and simple conf). Please clarify whether this is enough, or rather you ask me to support N providers. I don't want to write a "new feature" and then have someone say that we have a problem to introduce "new feature" in hadoop-1.
          Hide
          Avner BenHanoch added a comment -

          Additionally, as I wrote once, currently, there is no request and no use case for N providers. Hence, do we really want that?

          Show
          Avner BenHanoch added a comment - Additionally, as I wrote once, currently, there is no request and no use case for N providers. Hence, do we really want that?
          Hide
          Alejandro Abdelnur added a comment -

          Regarding 'new features in Hadoop-1'. Small or big, this is a new feature and it should be treated as such. I'm all for having this in Hadoop 1. If you want I can start the discussion in common-dev@.

          Regarding "Again, throughout the lifetime of this JIRA issue,...", I see different ways this can be done:

          • Keep everything in the same JIRA (as it is now) and wait till the whole patch is ready
          • Break the JIRA in 2 subtasks, consumer and producer side
            • Do it in branch-1 directly
            • Do it in a dev branch (seems an overkill)

          I'm OK with any approach, your call.

          Regarding "default implementation already implements the producer API".

          Ahh, missed that because the initialize method it is not used.

          With some minor tweaks to your patch I think we could get things done in a simple way:

          • Add to the TT a 'public Server getHttpServer()' method
          • In the TT constructor, where the MapOutputServlet is added to the HttpServer 'server', remove that line and discover, instantiate and initialize the provider plugin.
          • Don't make the MapOutputServlet to extends the provider interface.
          • The default provider should be a class that simply adds the MapOutputServlet to the server via the TT.getHttpServer() method.
          • Remove the logic to instantiate a custom single provider plugin.

          A provider multiplexor would be a very simple class, something along the following lines:

          public class MultiShuffleProviderPlugin implements ShuffleProviderPlugin {
            public static final String PLUGIN_CLASSES = "hadoop.mapreduce.multi.shuffle.provider.classes";
          
            private ShuffleProviderPlugin[] plugins;
          
            public void initialize(TaskTracker tt) {
              Configuration conf = tt.getJobConf();
              Class[] klasses = conf.getClasses(PLUGIN_CLASSES, DefaultShuffleProvider.class);
              //LOG INFO list of plugin classes
              plugins = new ShuffleProviderPlugin[klasses.length];
              for (int i = 0; i < klasses.length; i++) {
                plugins[i] = ReflectionUtils.newInstance(klasses[i], conf);
              }
              for (ShuffleProviderPlugin plugin : plugins) {
                plugin.initialize(tt);
              }
            }
          
            public void destroy() {
              if (plugins != null) {
                for (ShuffleProviderPlugin plugin : plugins) {
                  try {
                    plugin.destroy();
                  } catch (Throwable ex) {
                    //LOG WARN and ignore exception
                  }
                }
              }
            }
          
          
          }
          

          And the default provider class would be:

          
          public static class DefaultShuffleProviderPlugin implements ShuffleProviderPlugin {
          
            public void initialize(TaskTracker tt) {
                tt.getHttpServer().addInternalServlet("mapOutput", "/mapOutput", MapOutputServlet.class);
            }
          
            public void destroy() {
            }
          
          
          }
          
          Show
          Alejandro Abdelnur added a comment - Regarding 'new features in Hadoop-1'. Small or big, this is a new feature and it should be treated as such. I'm all for having this in Hadoop 1. If you want I can start the discussion in common-dev@. Regarding "Again, throughout the lifetime of this JIRA issue,...", I see different ways this can be done: Keep everything in the same JIRA (as it is now) and wait till the whole patch is ready Break the JIRA in 2 subtasks, consumer and producer side Do it in branch-1 directly Do it in a dev branch (seems an overkill) I'm OK with any approach, your call. Regarding "default implementation already implements the producer API". Ahh, missed that because the initialize method it is not used. With some minor tweaks to your patch I think we could get things done in a simple way: Add to the TT a 'public Server getHttpServer()' method In the TT constructor, where the MapOutputServlet is added to the HttpServer 'server', remove that line and discover, instantiate and initialize the provider plugin. Don't make the MapOutputServlet to extends the provider interface. The default provider should be a class that simply adds the MapOutputServlet to the server via the TT.getHttpServer() method. Remove the logic to instantiate a custom single provider plugin. A provider multiplexor would be a very simple class, something along the following lines: public class MultiShuffleProviderPlugin implements ShuffleProviderPlugin { public static final String PLUGIN_CLASSES = "hadoop.mapreduce.multi.shuffle.provider.classes" ; private ShuffleProviderPlugin[] plugins; public void initialize(TaskTracker tt) { Configuration conf = tt.getJobConf(); Class [] klasses = conf.getClasses(PLUGIN_CLASSES, DefaultShuffleProvider.class); //LOG INFO list of plugin classes plugins = new ShuffleProviderPlugin[klasses.length]; for ( int i = 0; i < klasses.length; i++) { plugins[i] = ReflectionUtils.newInstance(klasses[i], conf); } for (ShuffleProviderPlugin plugin : plugins) { plugin.initialize(tt); } } public void destroy() { if (plugins != null ) { for (ShuffleProviderPlugin plugin : plugins) { try { plugin.destroy(); } catch (Throwable ex) { //LOG WARN and ignore exception } } } } } And the default provider class would be: public static class DefaultShuffleProviderPlugin implements ShuffleProviderPlugin { public void initialize(TaskTracker tt) { tt.getHttpServer().addInternalServlet( "mapOutput" , "/mapOutput" , MapOutputServlet.class); } public void destroy() { } }
          Hide
          Avner BenHanoch added a comment -

          Hi Alejandro,

          Thanks for your comprehensive answer and code. It is important for me to agree with you on all the details before I go to implementation.

          Kindly, please let me know if you agree on all the bullets below:

          1. I’ll Keep consumer & producer in the same JIRA and submit one patch when all is ready (since this JIRA issue deals with generic shuffle service as set of two plugins). I’ll do my best to do it as soon as possible within few days.
          2. The consumer will be according to all our agreements so far
          3. The producer will be according to your code and remarks.
          4. Also, I understand that in "... remove that line and discover, instantiate and initialize the provider plugin" you simply mean "... remove that line and call multiShuffleProviderPlugin.initialize()"
          5. In addition, I’ll move the current call shuffleConsumerPlugin.destroy() from TaskTracker.close() method into TaskTracker.shutdown() method – this will match the move of shuffleConsumerPlugin.initialize into TT CTOR.

          Thanks,
          Avner

          Show
          Avner BenHanoch added a comment - Hi Alejandro, Thanks for your comprehensive answer and code. It is important for me to agree with you on all the details before I go to implementation. Kindly, please let me know if you agree on all the bullets below: I’ll Keep consumer & producer in the same JIRA and submit one patch when all is ready (since this JIRA issue deals with generic shuffle service as set of two plugins). I’ll do my best to do it as soon as possible within few days. The consumer will be according to all our agreements so far The producer will be according to your code and remarks. Also, I understand that in "... remove that line and discover, instantiate and initialize the provider plugin" you simply mean "... remove that line and call multiShuffleProviderPlugin.initialize()" In addition, I’ll move the current call shuffleConsumerPlugin.destroy() from TaskTracker.close() method into TaskTracker.shutdown() method – this will match the move of shuffleConsumerPlugin.initialize into TT CTOR. Thanks, Avner
          Hide
          Alejandro Abdelnur added a comment -

          Avner, sounds good. We can do another review iteration on your updated patch, it will be easier looking at concrete code. Thx

          Show
          Alejandro Abdelnur added a comment - Avner, sounds good. We can do another review iteration on your updated patch, it will be easier looking at concrete code. Thx
          Hide
          Hudson added a comment -

          Integrated in Hadoop-trunk-Commit #3282 (See https://builds.apache.org/job/Hadoop-trunk-Commit/3282/)
          Amending MR CHANGES.txt to reflect that MAPREDUCE-4049/4809/4807/4808 are in branch-2 (Revision 1438799)

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

          • /hadoop/common/trunk/hadoop-mapreduce-project/CHANGES.txt
          Show
          Hudson added a comment - Integrated in Hadoop-trunk-Commit #3282 (See https://builds.apache.org/job/Hadoop-trunk-Commit/3282/ ) Amending MR CHANGES.txt to reflect that MAPREDUCE-4049 /4809/4807/4808 are in branch-2 (Revision 1438799) Result = SUCCESS tucu : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1438799 Files : /hadoop/common/trunk/hadoop-mapreduce-project/CHANGES.txt
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Yarn-trunk #108 (See https://builds.apache.org/job/Hadoop-Yarn-trunk/108/)
          Amending MR CHANGES.txt to reflect that MAPREDUCE-4049/4809/4807/4808 are in branch-2 (Revision 1438799)

          Result = FAILURE
          tucu : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1438799
          Files :

          • /hadoop/common/trunk/hadoop-mapreduce-project/CHANGES.txt
          Show
          Hudson added a comment - Integrated in Hadoop-Yarn-trunk #108 (See https://builds.apache.org/job/Hadoop-Yarn-trunk/108/ ) Amending MR CHANGES.txt to reflect that MAPREDUCE-4049 /4809/4807/4808 are in branch-2 (Revision 1438799) Result = FAILURE tucu : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1438799 Files : /hadoop/common/trunk/hadoop-mapreduce-project/CHANGES.txt
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Hdfs-trunk #1297 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk/1297/)
          Amending MR CHANGES.txt to reflect that MAPREDUCE-4049/4809/4807/4808 are in branch-2 (Revision 1438799)

          Result = FAILURE
          tucu : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1438799
          Files :

          • /hadoop/common/trunk/hadoop-mapreduce-project/CHANGES.txt
          Show
          Hudson added a comment - Integrated in Hadoop-Hdfs-trunk #1297 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk/1297/ ) Amending MR CHANGES.txt to reflect that MAPREDUCE-4049 /4809/4807/4808 are in branch-2 (Revision 1438799) Result = FAILURE tucu : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1438799 Files : /hadoop/common/trunk/hadoop-mapreduce-project/CHANGES.txt
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Mapreduce-trunk #1325 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk/1325/)
          Amending MR CHANGES.txt to reflect that MAPREDUCE-4049/4809/4807/4808 are in branch-2 (Revision 1438799)

          Result = FAILURE
          tucu : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1438799
          Files :

          • /hadoop/common/trunk/hadoop-mapreduce-project/CHANGES.txt
          Show
          Hudson added a comment - Integrated in Hadoop-Mapreduce-trunk #1325 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk/1325/ ) Amending MR CHANGES.txt to reflect that MAPREDUCE-4049 /4809/4807/4808 are in branch-2 (Revision 1438799) Result = FAILURE tucu : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1438799 Files : /hadoop/common/trunk/hadoop-mapreduce-project/CHANGES.txt
          Hide
          Alejandro Abdelnur added a comment -

          Avner, as this is a new feature we should have the corresponding release notes. Please let me know if you want me to take care of it. Thx

          Show
          Alejandro Abdelnur added a comment - Avner, as this is a new feature we should have the corresponding release notes. Please let me know if you want me to take care of it. Thx
          Hide
          Avner BenHanoch added a comment -

          Alejandro, thanks for consulting me. I just added a note. Feel free to edit if needed.

          Show
          Avner BenHanoch added a comment - Alejandro, thanks for consulting me. I just added a note. Feel free to edit if needed.
          Hide
          Avner BenHanoch added a comment -

          Hi Alejadro,

          I attached a patch for branch-1 that addresses all your comments.

          Cheers,
          Avner

          Show
          Avner BenHanoch added a comment - Hi Alejadro, I attached a patch for branch-1 that addresses all your comments. Cheers, Avner
          Hide
          Alejandro Abdelnur added a comment -

          Hi Avner, patch looks good, just a few minor things:

          • ReduceTask.java: IOException message " - The reduce copier failed" should be something like " - The ShuffleConsumerPluging '" + clazz.getSimpleName() + "' failed"
          • ShuffleConsumerPlugin.java: unused imports JobConf & Task
          • ShuffleProviderPlugin.java: stability/audience annotations are missing
          • MultiShuffleProviderPlugin.java: log message on exception has typo, 'instanciating', it should be 'instantiating'.
          • MultiShuffleProviderPlugin.java: this class can be made private, the constant should be bubbled up to the TaskTracker. (I like this tweak rather than exposing it as a plugin).
          • TaskTracker.java: new method getJobConf(JobID) should be synchronized because the TreeMap runningJobs is not threadsafe. BTW, why this new method? I assume you need to access a job specific jobConf from the provider plugin? What is the use case for doing this?

          After this I think it is ready. Have you run all testcases successfully to check if there are any regressions?

          Do you want to start the discussion in the alias about adding new features to branch-1 or you want me to start it.

          Show
          Alejandro Abdelnur added a comment - Hi Avner, patch looks good, just a few minor things: ReduceTask.java: IOException message " - The reduce copier failed" should be something like " - The ShuffleConsumerPluging '" + clazz.getSimpleName() + "' failed" ShuffleConsumerPlugin.java: unused imports JobConf & Task ShuffleProviderPlugin.java: stability/audience annotations are missing MultiShuffleProviderPlugin.java: log message on exception has typo, 'instanciating', it should be 'instantiating'. MultiShuffleProviderPlugin.java: this class can be made private, the constant should be bubbled up to the TaskTracker. (I like this tweak rather than exposing it as a plugin). TaskTracker.java: new method getJobConf(JobID) should be synchronized because the TreeMap runningJobs is not threadsafe. BTW, why this new method? I assume you need to access a job specific jobConf from the provider plugin? What is the use case for doing this? After this I think it is ready. Have you run all testcases successfully to check if there are any regressions? Do you want to start the discussion in the alias about adding new features to branch-1 or you want me to start it.
          Hide
          Arun C Murthy added a comment -

          Sorry, just started looking at branch-1 patch. Apologies for being late.

          I'm worried about passing in the entire ReduceTask to the shuffle plugin - we should pass in a limited context...

          Show
          Arun C Murthy added a comment - Sorry, just started looking at branch-1 patch. Apologies for being late. I'm worried about passing in the entire ReduceTask to the shuffle plugin - we should pass in a limited context...
          Hide
          Arun C Murthy added a comment -

          I think it's fine to add new features to branch-1. We just need to be structured about it:

          1. Ensure all features are in branch-2/trunk
          2. Put features in a feature branch (1.2 or 1.3) but not in bug-fix release (1.2.1 or 1.3.2).
          Show
          Arun C Murthy added a comment - I think it's fine to add new features to branch-1. We just need to be structured about it: Ensure all features are in branch-2/trunk Put features in a feature branch (1.2 or 1.3) but not in bug-fix release (1.2.1 or 1.3.2).
          Hide
          Avner BenHanoch added a comment -

          Hi Alejadro & Arun,

          Thank you for your review and all your comments. I appreciate your help and responsiveness with my issue.

          I would like to say a few comments/answers before the patch is concluded:

          1. Alejandro - getJobConf(JobID) is needed for any ShuffleProvider. The provider needs it for determining username and runAsUsername. username is needed for determining the location in disk of the MOF and Index files. runAsUsername is needed for reading the above files with the right privileges.

          2. Alejandro – The answer for your question about the tests is - YES. I did run all smoke & commit tests successfully.

          3. Arun - I have no problem with your request for not passing the entire ReduceTask. I am only a bit worried about initing ShuffleConsumerPlugin with arguments such as getPartition() and getJobTokenSecret(). The reason is that at least theoretically it is possible to change partition/jobTokenSecret after the shuffleConsumerPlugin was initiated. Hence, I need your approval for that.
          Additionally, please notice that in hadoop-trunk we do pass the entire ReduceTask to the ShuffleConsumerPlugin. (Also, in hadoop-1 we always passed ReduceTask. I think that with the last patch it is highlighted because we made ReduceCopier a static class which required specifying explicitly reduceTask.XXX in about 75 different places).
          Bottom line, Arun, please let me know if you are still worried about passing the entire ReduceTask to the shuffle plugin.

          thank you,
          Avner

          Show
          Avner BenHanoch added a comment - Hi Alejadro & Arun, Thank you for your review and all your comments. I appreciate your help and responsiveness with my issue. I would like to say a few comments/answers before the patch is concluded: 1. Alejandro - getJobConf(JobID) is needed for any ShuffleProvider. The provider needs it for determining username and runAsUsername . username is needed for determining the location in disk of the MOF and Index files. runAsUsername is needed for reading the above files with the right privileges. 2. Alejandro – The answer for your question about the tests is - YES. I did run all smoke & commit tests successfully. 3. Arun - I have no problem with your request for not passing the entire ReduceTask. I am only a bit worried about initing ShuffleConsumerPlugin with arguments such as getPartition() and getJobTokenSecret() . The reason is that at least theoretically it is possible to change partition/jobTokenSecret after the shuffleConsumerPlugin was initiated. Hence, I need your approval for that. Additionally, please notice that in hadoop-trunk we do pass the entire ReduceTask to the ShuffleConsumerPlugin. (Also, in hadoop-1 we always passed ReduceTask. I think that with the last patch it is highlighted because we made ReduceCopier a static class which required specifying explicitly reduceTask.XXX in about 75 different places). Bottom line, Arun, please let me know if you are still worried about passing the entire ReduceTask to the shuffle plugin. thank you, Avner
          Hide
          Avner BenHanoch added a comment -

          Arun, Alejadro,
          I attached new patch for branch-1 that addresses all your comments.
          I am still passing ReduceTask to the plugin according to my explanation in the previous comment.
          Cheers,
          Avner

          Show
          Avner BenHanoch added a comment - Arun, Alejadro, I attached new patch for branch-1 that addresses all your comments. I am still passing ReduceTask to the plugin according to my explanation in the previous comment. Cheers, Avner
          Hide
          Alejandro Abdelnur added a comment -

          +1 from my side, regarding passing the ReduceTask, IMO, as discussed before, these are unstable APIs enabling functionality in an experimental way. Creating the right abstraction/API will require significant changes and we are not attempting to do that right now. Because of this, I think is OK. Arun, what is your take?

          Last, while we all 3 agree to commit this to branch-1. Before we do so, I want to have the discussion in the mapred-dev@ so everybody is on the same page and to avoid issues now or in the future.

          Show
          Alejandro Abdelnur added a comment - +1 from my side, regarding passing the ReduceTask, IMO, as discussed before, these are unstable APIs enabling functionality in an experimental way. Creating the right abstraction/API will require significant changes and we are not attempting to do that right now. Because of this, I think is OK. Arun, what is your take? Last, while we all 3 agree to commit this to branch-1. Before we do so, I want to have the discussion in the mapred-dev@ so everybody is on the same page and to avoid issues now or in the future.
          Hide
          Alejandro Abdelnur added a comment -

          Shortening release note to one liner. Detail documentation being introduced with MAPREDUCE-4977.

          Show
          Alejandro Abdelnur added a comment - Shortening release note to one liner. Detail documentation being introduced with MAPREDUCE-4977 .
          Hide
          Alejandro Abdelnur added a comment -

          Avner, would you please review the documentation, MAPREDUCE-4977, for correctness? THX

          Show
          Alejandro Abdelnur added a comment - Avner, would you please review the documentation, MAPREDUCE-4977 , for correctness? THX
          Hide
          Avner BenHanoch added a comment -

          Hi Arun,

          Alejandro and I feel comfortable with the current branch-1 patch as we explained above.
          Please let me know if you still prefer that I'll create a patch without passing the entire ReduceTask to ShuffleConsumerPlugin.
          I have no problem with any interface that is accepted by you and Alejandro.

          Thanks,
          Avner

          Show
          Avner BenHanoch added a comment - Hi Arun, Alejandro and I feel comfortable with the current branch-1 patch as we explained above. Please let me know if you still prefer that I'll create a patch without passing the entire ReduceTask to ShuffleConsumerPlugin. I have no problem with any interface that is accepted by you and Alejandro. Thanks, Avner
          Hide
          Avner BenHanoch added a comment -

          Hi Arun, and Alejandro, and anyone else,

          I would like to revive the branch-1 patch in this JIRA issue and submit it to the updated branch-1.

          This patch was already reviewed by Alejandro and got +1 in 05/Feb/13

          Please let me know what else is needed for closing this story.

          Thanks,
          Avner

          Show
          Avner BenHanoch added a comment - Hi Arun, and Alejandro, and anyone else, I would like to revive the branch-1 patch in this JIRA issue and submit it to the updated branch-1. This patch was already reviewed by Alejandro and got +1 in 05/Feb/13 Please let me know what else is needed for closing this story. Thanks, Avner
          Hide
          Alejandro Abdelnur added a comment -

          Avner BenHanoch, sorry for the delay. I've just tried applying the patch to branch-1 and there is one hunk failing, it seems a trivial rebase. Mind rebasing the patch to current HEAD of branch-1? Once you do that it can go in.

          Show
          Alejandro Abdelnur added a comment - Avner BenHanoch , sorry for the delay. I've just tried applying the patch to branch-1 and there is one hunk failing, it seems a trivial rebase. Mind rebasing the patch to current HEAD of branch-1? Once you do that it can go in.
          Hide
          Alejandro Abdelnur added a comment -

          Avner BenHanoch, never mind, seems some whitespace issue, just run all testcases successfully (no test-patch for branch-1). Committing momentarily.

          Show
          Alejandro Abdelnur added a comment - Avner BenHanoch , never mind, seems some whitespace issue, just run all testcases successfully (no test-patch for branch-1). Committing momentarily.
          Hide
          Alejandro Abdelnur added a comment -

          Thanks Avner. Committed to branch-1.

          Show
          Alejandro Abdelnur added a comment - Thanks Avner. Committed to branch-1.
          Hide
          Avner BenHanoch added a comment -

          Thanks Alejandro,

          You are faster than a rocket!

          Avner

          Show
          Avner BenHanoch added a comment - Thanks Alejandro, You are faster than a rocket! Avner

            People

            • Assignee:
              Avner BenHanoch
              Reporter:
              Avner BenHanoch
            • Votes:
              9 Vote for this issue
              Watchers:
              58 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development