Uploaded image for project: 'Hadoop Map/Reduce'
  1. Hadoop Map/Reduce
  2. MAPREDUCE-6870

Add configuration for MR job to finish when all reducers are complete (even with unfinished mappers)

    Details

    • Type: Improvement
    • Status: Resolved
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: 2.6.1
    • Fix Version/s: 3.0.0-beta1
    • Component/s: None
    • Labels:
      None
    • Target Version/s:
    • Hadoop Flags:
      Incompatible change, Reviewed
    • Release Note:
      Hide
      Enables mapreduce.job.finish-when-all-reducers-done by default. With this enabled, a MapReduce job will complete as soon as all of its reducers are complete, even if some mappers are still running. This can occur if a mapper was relaunched after node failure but the relaunched task's output is not actually needed. Previously the job would wait for all mappers to complete.
      Show
      Enables mapreduce.job.finish-when-all-reducers-done by default. With this enabled, a MapReduce job will complete as soon as all of its reducers are complete, even if some mappers are still running. This can occur if a mapper was relaunched after node failure but the relaunched task's output is not actually needed. Previously the job would wait for all mappers to complete.

      Description

      Even with MAPREDUCE-5817, there could still be cases where mappers get scheduled before all reducers are complete, but those mappers run for long time, even after all reducers are complete. This could hurt the performance of large MR jobs.

      In some cases, mappers don't have any materialize-able outcome other than providing intermediate data to reducers. In that case, the job owner should have the config option to finish the job once all reducers are complete.

      1. MAPREDUCE-6870-001.patch
        16 kB
        Peter Bacsko
      2. MAPREDUCE-6870-002.patch
        11 kB
        Peter Bacsko
      3. MAPREDUCE-6870-003.patch
        15 kB
        Peter Bacsko
      4. MAPREDUCE-6870-004.patch
        12 kB
        Peter Bacsko
      5. MAPREDUCE-6870-005.patch
        12 kB
        Peter Bacsko
      6. MAPREDUCE-6870-006.patch
        12 kB
        Peter Bacsko
      7. MAPREDUCE-6870-007.patch
        12 kB
        Peter Bacsko

        Issue Links

          Activity

          Hide
          pbacsko Peter Bacsko added a comment -

          Uploaded the first version. I verified it on a 4-node cluster.

          Show
          pbacsko Peter Bacsko added a comment - Uploaded the first version. I verified it on a 4-node cluster.
          Hide
          hadoopqa Hadoop QA added a comment -
          +1 overall



          Vote Subsystem Runtime Comment
          0 reexec 0m 20s Docker mode activated.
          +1 @author 0m 0s The patch does not contain any @author tags.
          +1 test4tests 0m 0s The patch appears to include 2 new or modified test files.
          0 mvndep 0m 9s Maven dependency ordering for branch
          +1 mvninstall 16m 4s trunk passed
          +1 compile 2m 13s trunk passed
          +1 checkstyle 0m 32s trunk passed
          +1 mvnsite 1m 8s trunk passed
          +1 mvneclipse 0m 38s trunk passed
          +1 findbugs 1m 44s trunk passed
          +1 javadoc 0m 42s trunk passed
          0 mvndep 0m 9s Maven dependency ordering for patch
          +1 mvninstall 0m 59s the patch passed
          +1 compile 2m 12s the patch passed
          +1 javac 2m 12s the patch passed
          +1 checkstyle 0m 32s the patch passed
          +1 mvnsite 1m 7s the patch passed
          +1 mvneclipse 0m 34s the patch passed
          +1 whitespace 0m 0s The patch has no whitespace issues.
          +1 findbugs 2m 14s the patch passed
          +1 javadoc 0m 43s the patch passed
          +1 unit 3m 38s hadoop-mapreduce-client-core in the patch passed.
          +1 unit 10m 46s hadoop-mapreduce-client-app in the patch passed.
          +1 asflicense 0m 19s The patch does not generate ASF License warnings.
          47m 40s



          Subsystem Report/Notes
          Docker Image:yetus/hadoop:14b5c93
          JIRA Issue MAPREDUCE-6870
          JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12875054/MAPREDUCE-6870-001.patch
          Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle
          uname Linux 0e0fcc4e7f1a 4.4.0-43-generic #63-Ubuntu SMP Wed Oct 12 13:48:03 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux
          Build tool maven
          Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh
          git revision trunk / 16c8dbd
          Default Java 1.8.0_131
          findbugs v3.1.0-RC1
          Test Results https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/7007/testReport/
          modules C: hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-core hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-app U: hadoop-mapreduce-project/hadoop-mapreduce-client
          Console output https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/7007/console
          Powered by Apache Yetus 0.4.0 http://yetus.apache.org

          This message was automatically generated.

          Show
          hadoopqa Hadoop QA added a comment - +1 overall Vote Subsystem Runtime Comment 0 reexec 0m 20s Docker mode activated. +1 @author 0m 0s The patch does not contain any @author tags. +1 test4tests 0m 0s The patch appears to include 2 new or modified test files. 0 mvndep 0m 9s Maven dependency ordering for branch +1 mvninstall 16m 4s trunk passed +1 compile 2m 13s trunk passed +1 checkstyle 0m 32s trunk passed +1 mvnsite 1m 8s trunk passed +1 mvneclipse 0m 38s trunk passed +1 findbugs 1m 44s trunk passed +1 javadoc 0m 42s trunk passed 0 mvndep 0m 9s Maven dependency ordering for patch +1 mvninstall 0m 59s the patch passed +1 compile 2m 12s the patch passed +1 javac 2m 12s the patch passed +1 checkstyle 0m 32s the patch passed +1 mvnsite 1m 7s the patch passed +1 mvneclipse 0m 34s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. +1 findbugs 2m 14s the patch passed +1 javadoc 0m 43s the patch passed +1 unit 3m 38s hadoop-mapreduce-client-core in the patch passed. +1 unit 10m 46s hadoop-mapreduce-client-app in the patch passed. +1 asflicense 0m 19s The patch does not generate ASF License warnings. 47m 40s Subsystem Report/Notes Docker Image:yetus/hadoop:14b5c93 JIRA Issue MAPREDUCE-6870 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12875054/MAPREDUCE-6870-001.patch Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle uname Linux 0e0fcc4e7f1a 4.4.0-43-generic #63-Ubuntu SMP Wed Oct 12 13:48:03 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh git revision trunk / 16c8dbd Default Java 1.8.0_131 findbugs v3.1.0-RC1 Test Results https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/7007/testReport/ modules C: hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-core hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-app U: hadoop-mapreduce-project/hadoop-mapreduce-client Console output https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/7007/console Powered by Apache Yetus 0.4.0 http://yetus.apache.org This message was automatically generated.
          Hide
          haibochen Haibo Chen added a comment -

          Thanks Peter Bacsko for the patch! Doesn't the fact all reducers have completed always indicate the job is ready to finish? If so, I don't think we need to add another configuration to handle such cases.

          Following that, we could move what's inside in preemptMappersIfNecessary() to checkJobAfterTaskCompletion() (after check for job failure and before job.checkReadyForCommit()), because it is not preempting running mappers but just a part of checking whether a job is ready to finish.

          Plus, instead of killing taskattempts, we could just send kill events to map tasks which will in turn kill their individual attempts. Thus, we do not need setNoMoreAttempts() any more.

          Show
          haibochen Haibo Chen added a comment - Thanks Peter Bacsko for the patch! Doesn't the fact all reducers have completed always indicate the job is ready to finish? If so, I don't think we need to add another configuration to handle such cases. Following that, we could move what's inside in preemptMappersIfNecessary() to checkJobAfterTaskCompletion() (after check for job failure and before job.checkReadyForCommit()), because it is not preempting running mappers but just a part of checking whether a job is ready to finish. Plus, instead of killing taskattempts, we could just send kill events to map tasks which will in turn kill their individual attempts. Thus, we do not need setNoMoreAttempts() any more.
          Hide
          pbacsko Peter Bacsko added a comment -

          Thanks Haibo Chen - the reason I added the config toggle is because the JIRA description says: "the job owner should have the config option to finish the job once all reducers are complete". It is telling me that this behavior should be configurable. I don't know whether this should be enabled by default or not (probably yes).

          I'll do the modifications you requested.

          Show
          pbacsko Peter Bacsko added a comment - Thanks Haibo Chen - the reason I added the config toggle is because the JIRA description says: "the job owner should have the config option to finish the job once all reducers are complete". It is telling me that this behavior should be configurable. I don't know whether this should be enabled by default or not (probably yes). I'll do the modifications you requested.
          Hide
          hadoopqa Hadoop QA added a comment -
          +1 overall



          Vote Subsystem Runtime Comment
          0 reexec 0m 17s Docker mode activated.
          +1 @author 0m 0s The patch does not contain any @author tags.
          +1 test4tests 0m 0s The patch appears to include 1 new or modified test files.
          0 mvndep 0m 10s Maven dependency ordering for branch
          +1 mvninstall 16m 26s trunk passed
          +1 compile 2m 21s trunk passed
          +1 checkstyle 0m 33s trunk passed
          +1 mvnsite 1m 9s trunk passed
          +1 mvneclipse 0m 38s trunk passed
          +1 findbugs 1m 41s trunk passed
          +1 javadoc 0m 42s trunk passed
          0 mvndep 0m 9s Maven dependency ordering for patch
          +1 mvninstall 0m 59s the patch passed
          +1 compile 2m 14s the patch passed
          +1 javac 2m 14s the patch passed
          +1 checkstyle 0m 31s the patch passed
          +1 mvnsite 1m 5s the patch passed
          +1 mvneclipse 0m 38s the patch passed
          +1 whitespace 0m 0s The patch has no whitespace issues.
          +1 findbugs 2m 16s the patch passed
          +1 javadoc 0m 45s the patch passed
          +1 unit 3m 29s hadoop-mapreduce-client-core in the patch passed.
          +1 unit 11m 30s hadoop-mapreduce-client-app in the patch passed.
          +1 asflicense 0m 23s The patch does not generate ASF License warnings.
          49m 1s



          Subsystem Report/Notes
          Docker Image:yetus/hadoop:14b5c93
          JIRA Issue MAPREDUCE-6870
          JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12875773/MAPREDUCE-6870-002.patch
          Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle
          uname Linux b3d8b3efa3b2 4.4.0-43-generic #63-Ubuntu SMP Wed Oct 12 13:48:03 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux
          Build tool maven
          Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh
          git revision trunk / a180ba4
          Default Java 1.8.0_131
          findbugs v3.1.0-RC1
          Test Results https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/7017/testReport/
          modules C: hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-core hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-app U: hadoop-mapreduce-project/hadoop-mapreduce-client
          Console output https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/7017/console
          Powered by Apache Yetus 0.4.0 http://yetus.apache.org

          This message was automatically generated.

          Show
          hadoopqa Hadoop QA added a comment - +1 overall Vote Subsystem Runtime Comment 0 reexec 0m 17s Docker mode activated. +1 @author 0m 0s The patch does not contain any @author tags. +1 test4tests 0m 0s The patch appears to include 1 new or modified test files. 0 mvndep 0m 10s Maven dependency ordering for branch +1 mvninstall 16m 26s trunk passed +1 compile 2m 21s trunk passed +1 checkstyle 0m 33s trunk passed +1 mvnsite 1m 9s trunk passed +1 mvneclipse 0m 38s trunk passed +1 findbugs 1m 41s trunk passed +1 javadoc 0m 42s trunk passed 0 mvndep 0m 9s Maven dependency ordering for patch +1 mvninstall 0m 59s the patch passed +1 compile 2m 14s the patch passed +1 javac 2m 14s the patch passed +1 checkstyle 0m 31s the patch passed +1 mvnsite 1m 5s the patch passed +1 mvneclipse 0m 38s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. +1 findbugs 2m 16s the patch passed +1 javadoc 0m 45s the patch passed +1 unit 3m 29s hadoop-mapreduce-client-core in the patch passed. +1 unit 11m 30s hadoop-mapreduce-client-app in the patch passed. +1 asflicense 0m 23s The patch does not generate ASF License warnings. 49m 1s Subsystem Report/Notes Docker Image:yetus/hadoop:14b5c93 JIRA Issue MAPREDUCE-6870 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12875773/MAPREDUCE-6870-002.patch Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle uname Linux b3d8b3efa3b2 4.4.0-43-generic #63-Ubuntu SMP Wed Oct 12 13:48:03 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh git revision trunk / a180ba4 Default Java 1.8.0_131 findbugs v3.1.0-RC1 Test Results https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/7017/testReport/ modules C: hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-core hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-app U: hadoop-mapreduce-project/hadoop-mapreduce-client Console output https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/7017/console Powered by Apache Yetus 0.4.0 http://yetus.apache.org This message was automatically generated.
          Hide
          pbacsko Peter Bacsko added a comment -
          Show
          pbacsko Peter Bacsko added a comment - ping Haibo Chen
          Hide
          shv Konstantin Shvachko added a comment -

          Moving target version to 2.7.5 due to 2.7.4 release.

          Show
          shv Konstantin Shvachko added a comment - Moving target version to 2.7.5 due to 2.7.4 release.
          Hide
          haibochen Haibo Chen added a comment -

          Thanks Peter Bacsko for reminding me. A few more comments after taking a close look.
          1) The new configuration is called mapreduce.job.map.preempt-on-reduce-finish, which is a little deceptive in that what we are really doing is to let the job finish. Preempting any running mapper is just part of doing that. How about we rename it to something like mapreduce.job.finish-when-all-reducers-done? Also add a static variable in MRJobConfig to represent its default value 'true' and documentation in mapred-default.xml for this new configuration.

          2) In preemptMappersIfNecessary(), we can reference job.mapTasks directly and get rid of the type check

          3) With the current flow, that is, we execute preemptMappersIfNecessary upon every task completion event, redundant T_KILL events can be generated. Say there are 3 mappers that are still running when all reducers are done, we would send 3 T_KILL events first. When one of the 3 mappers, whichever is the first, is killed, it triggers another Task completion event and we send 2 T_KILL events. In total, we'd send 3+2+1 events. I think we want to make sure we do preemption of running mappers only once.

          4) The statement in checkJobAfterTaskCompletion()

                if (job.preemptRestartedMappersOnReduceFinish) {
                  preemptMappersIfNecessary(job);
                }
          

          is hard to follow without referring to this jira. For readability, we can put this in a new method, maybe called checkReadyForCompletionWhenAllReducersDone(), inline whatever we have in preemptMappersIfNecessary(). Then have comments on this method explain what we are doing here and why.

          5) There are a few unused import statements in TestJobImpl. TestJobImpl.createSpiedTasks() is rather createSpiedMapTasks, so we can rename that. conf.set(MRJobConfig.PREEMPT_MAPPERS_ON_REDUCE_FINISH, Boolean.toString(killMappers)) can be replaced with conf.setBoolean(,).

          6) In the newly added unit test, we are just verifying that the mapper are killed. Similar to 1), we want to finish the job, so I think we should verify job completion first if our new configuration is set to true.

          Show
          haibochen Haibo Chen added a comment - Thanks Peter Bacsko for reminding me. A few more comments after taking a close look. 1) The new configuration is called mapreduce.job.map.preempt-on-reduce-finish, which is a little deceptive in that what we are really doing is to let the job finish. Preempting any running mapper is just part of doing that. How about we rename it to something like mapreduce.job.finish-when-all-reducers-done? Also add a static variable in MRJobConfig to represent its default value 'true' and documentation in mapred-default.xml for this new configuration. 2) In preemptMappersIfNecessary(), we can reference job.mapTasks directly and get rid of the type check 3) With the current flow, that is, we execute preemptMappersIfNecessary upon every task completion event, redundant T_KILL events can be generated. Say there are 3 mappers that are still running when all reducers are done, we would send 3 T_KILL events first. When one of the 3 mappers, whichever is the first, is killed, it triggers another Task completion event and we send 2 T_KILL events. In total, we'd send 3+2+1 events. I think we want to make sure we do preemption of running mappers only once. 4) The statement in checkJobAfterTaskCompletion() if (job.preemptRestartedMappersOnReduceFinish) { preemptMappersIfNecessary(job); } is hard to follow without referring to this jira. For readability, we can put this in a new method, maybe called checkReadyForCompletionWhenAllReducersDone(), inline whatever we have in preemptMappersIfNecessary(). Then have comments on this method explain what we are doing here and why. 5) There are a few unused import statements in TestJobImpl. TestJobImpl.createSpiedTasks() is rather createSpiedMapTasks, so we can rename that. conf.set(MRJobConfig.PREEMPT_MAPPERS_ON_REDUCE_FINISH, Boolean.toString(killMappers)) can be replaced with conf.setBoolean(,). 6) In the newly added unit test, we are just verifying that the mapper are killed. Similar to 1), we want to finish the job, so I think we should verify job completion first if our new configuration is set to true.
          Hide
          pbacsko Peter Bacsko added a comment -

          Thanks Haibo Chen, I addressed your comments.

          Things to check / consider:
          1. Variable names (is preemptMappersOnReduceFinish good?)
          2. Added a new method to MapTaskImpl with locking, which is probably not necessary but I felt it's better to have it anyway

          Show
          pbacsko Peter Bacsko added a comment - Thanks Haibo Chen , I addressed your comments. Things to check / consider: 1. Variable names (is preemptMappersOnReduceFinish good?) 2. Added a new method to MapTaskImpl with locking, which is probably not necessary but I felt it's better to have it anyway
          Hide
          hadoopqa Hadoop QA added a comment -
          +1 overall



          Vote Subsystem Runtime Comment
          0 reexec 0m 21s Docker mode activated.
          +1 @author 0m 0s The patch does not contain any @author tags.
          +1 test4tests 0m 0s The patch appears to include 1 new or modified test files.
          0 mvndep 0m 54s Maven dependency ordering for branch
          +1 mvninstall 14m 36s trunk passed
          +1 compile 1m 50s trunk passed
          +1 checkstyle 0m 31s trunk passed
          +1 mvnsite 0m 57s trunk passed
          +1 mvneclipse 0m 37s trunk passed
          +1 findbugs 1m 31s trunk passed
          +1 javadoc 0m 40s trunk passed
          0 mvndep 0m 8s Maven dependency ordering for patch
          +1 mvninstall 0m 50s the patch passed
          +1 compile 1m 45s the patch passed
          +1 javac 1m 45s the patch passed
          +1 checkstyle 0m 27s the patch passed
          +1 mvnsite 0m 54s the patch passed
          +1 mvneclipse 0m 30s the patch passed
          +1 whitespace 0m 0s The patch has no whitespace issues.
          +1 xml 0m 2s The patch has no ill-formed XML file.
          +1 findbugs 1m 42s the patch passed
          +1 javadoc 0m 35s the patch passed
          +1 unit 2m 45s hadoop-mapreduce-client-core in the patch passed.
          +1 unit 9m 6s hadoop-mapreduce-client-app in the patch passed.
          +1 asflicense 0m 17s The patch does not generate ASF License warnings.
          41m 55s



          Subsystem Report/Notes
          Docker Image:yetus/hadoop:14b5c93
          JIRA Issue MAPREDUCE-6870
          JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12880006/MAPREDUCE-6870-003.patch
          Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle xml
          uname Linux 9d9cb5dc9e02 3.13.0-117-generic #164-Ubuntu SMP Fri Apr 7 11:05:26 UTC 2017 x86_64 x86_64 x86_64 GNU/Linux
          Build tool maven
          Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh
          git revision trunk / f9139ac
          Default Java 1.8.0_131
          findbugs v3.1.0-RC1
          Test Results https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/7038/testReport/
          modules C: hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-core hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-app U: hadoop-mapreduce-project/hadoop-mapreduce-client
          Console output https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/7038/console
          Powered by Apache Yetus 0.4.0 http://yetus.apache.org

          This message was automatically generated.

          Show
          hadoopqa Hadoop QA added a comment - +1 overall Vote Subsystem Runtime Comment 0 reexec 0m 21s Docker mode activated. +1 @author 0m 0s The patch does not contain any @author tags. +1 test4tests 0m 0s The patch appears to include 1 new or modified test files. 0 mvndep 0m 54s Maven dependency ordering for branch +1 mvninstall 14m 36s trunk passed +1 compile 1m 50s trunk passed +1 checkstyle 0m 31s trunk passed +1 mvnsite 0m 57s trunk passed +1 mvneclipse 0m 37s trunk passed +1 findbugs 1m 31s trunk passed +1 javadoc 0m 40s trunk passed 0 mvndep 0m 8s Maven dependency ordering for patch +1 mvninstall 0m 50s the patch passed +1 compile 1m 45s the patch passed +1 javac 1m 45s the patch passed +1 checkstyle 0m 27s the patch passed +1 mvnsite 0m 54s the patch passed +1 mvneclipse 0m 30s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. +1 xml 0m 2s The patch has no ill-formed XML file. +1 findbugs 1m 42s the patch passed +1 javadoc 0m 35s the patch passed +1 unit 2m 45s hadoop-mapreduce-client-core in the patch passed. +1 unit 9m 6s hadoop-mapreduce-client-app in the patch passed. +1 asflicense 0m 17s The patch does not generate ASF License warnings. 41m 55s Subsystem Report/Notes Docker Image:yetus/hadoop:14b5c93 JIRA Issue MAPREDUCE-6870 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12880006/MAPREDUCE-6870-003.patch Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle xml uname Linux 9d9cb5dc9e02 3.13.0-117-generic #164-Ubuntu SMP Fri Apr 7 11:05:26 UTC 2017 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh git revision trunk / f9139ac Default Java 1.8.0_131 findbugs v3.1.0-RC1 Test Results https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/7038/testReport/ modules C: hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-core hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-app U: hadoop-mapreduce-project/hadoop-mapreduce-client Console output https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/7038/console Powered by Apache Yetus 0.4.0 http://yetus.apache.org This message was automatically generated.
          Hide
          haibochen Haibo Chen added a comment -

          1. Variable names (is preemptMappersOnReduceFinish good?)

          I thingk we can rename JobImplpreemptMappersOnReduceFinish, MRJobConfig.PREEMPT_MAPPERS_ON_REDUCE_FINISH and MRJobConfig.DEFAULT_PREEMPT_MAPPERS_ON_REDUCE_FINISH to convey more clearly that our intention is to finish the job from the users' perspective (because the configuration is user-facing). Also the description of mapreduce.job.finish-when-all-reducers-done in mapred-default.xml.

          2. Added a new method to MapTaskImpl with locking, which is probably not necessary but I felt it's better to have it anyway

          What I meant previously is that we are creating duplicated TA_KILL events in JobImpl.checkReadyForCompletionWhenAllReducersDone(). I am not sure adding MapTaskImp.preemptedDueToReducerFinish is going to help in this case, because we will still be sending duplicated TA_KILL events. I was proposing something like the following

              private void checkReadyForCompletionWhenAllReducersDone(JobImpl job) {
                if (job.preemptRestartedMappersOnReduceFinish) {
                  int totalReduces = job.getTotalReduces();
                  int completedReduces = job.getCompletedReduces();
                  // only if all reducers have finished and we have not sent TA_KILL events to running
                  // map tasks before
                  if (totalReduces > 0 && totalReduces == completedReduces && !inCompletingJob) {
                    for (TaskId mapTaskId : job.mapTasks) {
                      MapTaskImpl task = (MapTaskImpl) job.tasks.get(mapTaskId);
                      if (!task.isFinished() && !task.isPreemptedDueToReducerFinish()) {
                        LOG.info("Killing map task " + task.getID());
                        task.setPreemptedDueToReducerFinish(true);
                        job.eventHandler.handle(
                            new TaskEvent(task.getID(), TaskEventType.T_KILL));
                      }
                    }
                    inCompletingJob = true;
                  }
                }
          

          In TestJobImpl, can you explain to me what this code is doing

              // finish mappers - if the mapper is preempted, its state will be
              // KILLED, set by KillWaitAttemptKilledTransition
              for (TaskId taskId: job.tasks.keySet()) {
                TaskState state = killMappers ? TaskState.KILLED : TaskState.SUCCEEDED;
                if (taskId.getTaskType() == TaskType.MAP) {
                  job.handle(new JobTaskEvent(taskId, state));
                }
              }
          

          Also, do we expect the job the succeed even when killMappers is set to false? I'd expect if killMappers is false,
          The mappers will stay in RUNNING state, and thus hanging the job. In other words, the job should stay in
          RUNNING state (after we drain events in the dispatcher event queue). No?

          Show
          haibochen Haibo Chen added a comment - 1. Variable names (is preemptMappersOnReduceFinish good?) I thingk we can rename JobImplpreemptMappersOnReduceFinish, MRJobConfig.PREEMPT_MAPPERS_ON_REDUCE_FINISH and MRJobConfig.DEFAULT_PREEMPT_MAPPERS_ON_REDUCE_FINISH to convey more clearly that our intention is to finish the job from the users' perspective (because the configuration is user-facing). Also the description of mapreduce.job.finish-when-all-reducers-done in mapred-default.xml. 2. Added a new method to MapTaskImpl with locking, which is probably not necessary but I felt it's better to have it anyway What I meant previously is that we are creating duplicated TA_KILL events in JobImpl.checkReadyForCompletionWhenAllReducersDone(). I am not sure adding MapTaskImp.preemptedDueToReducerFinish is going to help in this case, because we will still be sending duplicated TA_KILL events. I was proposing something like the following private void checkReadyForCompletionWhenAllReducersDone(JobImpl job) { if (job.preemptRestartedMappersOnReduceFinish) { int totalReduces = job.getTotalReduces(); int completedReduces = job.getCompletedReduces(); // only if all reducers have finished and we have not sent TA_KILL events to running // map tasks before if (totalReduces > 0 && totalReduces == completedReduces && !inCompletingJob) { for (TaskId mapTaskId : job.mapTasks) { MapTaskImpl task = (MapTaskImpl) job.tasks.get(mapTaskId); if (!task.isFinished() && !task.isPreemptedDueToReducerFinish()) { LOG.info( "Killing map task " + task.getID()); task.setPreemptedDueToReducerFinish( true ); job.eventHandler.handle( new TaskEvent(task.getID(), TaskEventType.T_KILL)); } } inCompletingJob = true ; } } In TestJobImpl, can you explain to me what this code is doing // finish mappers - if the mapper is preempted, its state will be // KILLED, set by KillWaitAttemptKilledTransition for (TaskId taskId: job.tasks.keySet()) { TaskState state = killMappers ? TaskState.KILLED : TaskState.SUCCEEDED; if (taskId.getTaskType() == TaskType.MAP) { job.handle( new JobTaskEvent(taskId, state)); } } Also, do we expect the job the succeed even when killMappers is set to false? I'd expect if killMappers is false, The mappers will stay in RUNNING state, and thus hanging the job. In other words, the job should stay in RUNNING state (after we drain events in the dispatcher event queue). No?
          Hide
          pbacsko Peter Bacsko added a comment - - edited

          What's your suggestion to the variables?
          My ideas: finishJobWhenReducersDone, MRJobConfig.FINISH_JOB_WHEN_REDUCERS_DONE

          Preventing TA_KILL events: basically I just store a state information in each MapTaskImpl. But it's unnecessary since you can store this in a single variable after sending the kill events. So your approach is better.

          New test: in a real environment, certain events are coming from TaskImpl and TaskAttemptImpl. However in the tests, these are mocked inside JobImpl, so you have to generate them manually. To properly test the behavior of this change, it might make sense to use the real impl classes instead of mocks.

          Also, do we expect the job the succeed even when killMappers is set to false?

          Only if we send the completion events. If we don't, then of course it stays in RUNNING. I took the idea of finishing mappers/reducers from this test: https://github.com/apache/hadoop/blob/78b487bde175544ebe40e4dafab35569baa1d79e/hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-app/src/test/java/org/apache/hadoop/mapreduce/v2/app/job/impl/TestJobImpl.java#L597-L625

          Show
          pbacsko Peter Bacsko added a comment - - edited What's your suggestion to the variables? My ideas: finishJobWhenReducersDone , MRJobConfig.FINISH_JOB_WHEN_REDUCERS_DONE Preventing TA_KILL events: basically I just store a state information in each MapTaskImpl . But it's unnecessary since you can store this in a single variable after sending the kill events. So your approach is better. New test: in a real environment, certain events are coming from TaskImpl and TaskAttemptImpl . However in the tests, these are mocked inside JobImpl , so you have to generate them manually. To properly test the behavior of this change, it might make sense to use the real impl classes instead of mocks. Also, do we expect the job the succeed even when killMappers is set to false? Only if we send the completion events. If we don't, then of course it stays in RUNNING. I took the idea of finishing mappers/reducers from this test: https://github.com/apache/hadoop/blob/78b487bde175544ebe40e4dafab35569baa1d79e/hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-app/src/test/java/org/apache/hadoop/mapreduce/v2/app/job/impl/TestJobImpl.java#L597-L625
          Hide
          haibochen Haibo Chen added a comment -

          My ideas: finishJobWhenReducersDone, MRJobConfig.FINISH_JOB_WHEN_REDUCERS_DONE

          Looks good to me.

          However in the tests, these are mocked inside JobImpl, so you have to generate them manually

          I see. 'assertJobState(job, JobStateInternal.SUCCEEDED)' always succeeds regardless of whether we have the changes in JobImpl or not, so I'd argue that we could remove it along with the statements to finish mappers since they don't verify anything. Then we'd just have the assertions to verify kill events. Optionally, we can add a comment saying the reason why we are not verifying whether the job succeeds or not is that StubJob does not handle events internally.

          Show
          haibochen Haibo Chen added a comment - My ideas: finishJobWhenReducersDone, MRJobConfig.FINISH_JOB_WHEN_REDUCERS_DONE Looks good to me. However in the tests, these are mocked inside JobImpl, so you have to generate them manually I see. 'assertJobState(job, JobStateInternal.SUCCEEDED)' always succeeds regardless of whether we have the changes in JobImpl or not, so I'd argue that we could remove it along with the statements to finish mappers since they don't verify anything. Then we'd just have the assertions to verify kill events. Optionally, we can add a comment saying the reason why we are not verifying whether the job succeeds or not is that StubJob does not handle events internally.
          Hide
          hadoopqa Hadoop QA added a comment -
          -1 overall



          Vote Subsystem Runtime Comment
          0 reexec 0m 46s Docker mode activated.
                Prechecks
          +1 @author 0m 0s The patch does not contain any @author tags.
          +1 test4tests 0m 0s The patch appears to include 1 new or modified test files.
                trunk Compile Tests
          0 mvndep 1m 2s Maven dependency ordering for branch
          +1 mvninstall 15m 58s trunk passed
          +1 compile 1m 47s trunk passed
          +1 checkstyle 0m 38s trunk passed
          +1 mvnsite 0m 56s trunk passed
          +1 findbugs 1m 25s trunk passed
          +1 javadoc 0m 43s trunk passed
                Patch Compile Tests
          0 mvndep 0m 10s Maven dependency ordering for patch
          +1 mvninstall 1m 1s the patch passed
          +1 compile 1m 47s the patch passed
          +1 javac 1m 47s the patch passed
          -1 checkstyle 0m 42s hadoop-mapreduce-project/hadoop-mapreduce-client: The patch generated 13 new + 658 unchanged - 1 fixed = 671 total (was 659)
          +1 mvnsite 1m 6s the patch passed
          +1 whitespace 0m 0s The patch has no whitespace issues.
          +1 xml 0m 2s The patch has no ill-formed XML file.
          +1 findbugs 1m 50s the patch passed
          +1 javadoc 0m 34s the patch passed
                Other Tests
          +1 unit 2m 43s hadoop-mapreduce-client-core in the patch passed.
          +1 unit 8m 58s hadoop-mapreduce-client-app in the patch passed.
          +1 asflicense 0m 17s The patch does not generate ASF License warnings.
          43m 23s



          Subsystem Report/Notes
          Docker Image:yetus/hadoop:14b5c93
          JIRA Issue MAPREDUCE-6870
          JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12880853/MAPREDUCE-6870-004.patch
          Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle xml
          uname Linux d6ce2ea4a0af 3.13.0-117-generic #164-Ubuntu SMP Fri Apr 7 11:05:26 UTC 2017 x86_64 x86_64 x86_64 GNU/Linux
          Build tool maven
          Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh
          git revision trunk / 9891295
          Default Java 1.8.0_131
          findbugs v3.1.0-RC1
          checkstyle https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/7049/artifact/patchprocess/diff-checkstyle-hadoop-mapreduce-project_hadoop-mapreduce-client.txt
          Test Results https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/7049/testReport/
          modules C: hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-core hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-app U: hadoop-mapreduce-project/hadoop-mapreduce-client
          Console output https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/7049/console
          Powered by Apache Yetus 0.5.0 http://yetus.apache.org

          This message was automatically generated.

          Show
          hadoopqa Hadoop QA added a comment - -1 overall Vote Subsystem Runtime Comment 0 reexec 0m 46s Docker mode activated.       Prechecks +1 @author 0m 0s The patch does not contain any @author tags. +1 test4tests 0m 0s The patch appears to include 1 new or modified test files.       trunk Compile Tests 0 mvndep 1m 2s Maven dependency ordering for branch +1 mvninstall 15m 58s trunk passed +1 compile 1m 47s trunk passed +1 checkstyle 0m 38s trunk passed +1 mvnsite 0m 56s trunk passed +1 findbugs 1m 25s trunk passed +1 javadoc 0m 43s trunk passed       Patch Compile Tests 0 mvndep 0m 10s Maven dependency ordering for patch +1 mvninstall 1m 1s the patch passed +1 compile 1m 47s the patch passed +1 javac 1m 47s the patch passed -1 checkstyle 0m 42s hadoop-mapreduce-project/hadoop-mapreduce-client: The patch generated 13 new + 658 unchanged - 1 fixed = 671 total (was 659) +1 mvnsite 1m 6s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. +1 xml 0m 2s The patch has no ill-formed XML file. +1 findbugs 1m 50s the patch passed +1 javadoc 0m 34s the patch passed       Other Tests +1 unit 2m 43s hadoop-mapreduce-client-core in the patch passed. +1 unit 8m 58s hadoop-mapreduce-client-app in the patch passed. +1 asflicense 0m 17s The patch does not generate ASF License warnings. 43m 23s Subsystem Report/Notes Docker Image:yetus/hadoop:14b5c93 JIRA Issue MAPREDUCE-6870 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12880853/MAPREDUCE-6870-004.patch Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle xml uname Linux d6ce2ea4a0af 3.13.0-117-generic #164-Ubuntu SMP Fri Apr 7 11:05:26 UTC 2017 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh git revision trunk / 9891295 Default Java 1.8.0_131 findbugs v3.1.0-RC1 checkstyle https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/7049/artifact/patchprocess/diff-checkstyle-hadoop-mapreduce-project_hadoop-mapreduce-client.txt Test Results https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/7049/testReport/ modules C: hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-core hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-app U: hadoop-mapreduce-project/hadoop-mapreduce-client Console output https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/7049/console Powered by Apache Yetus 0.5.0 http://yetus.apache.org This message was automatically generated.
          Hide
          haibochen Haibo Chen added a comment -

          A few knits:
          1) Use block comment /* **/ for checkReadyForCompletionWhenAllReducersDone()
          2) We can avoid iterating over all map tasks if job. completingJob is true, that is,

          if (totalReduces > 0 && totalReduces == completedReduces) {
              if (!job.completingJob) {
                  for(task: mapTasks) {
                      kill if task is not finished.
                  }
                  job.completingJob = true;
              }
          }
          

          3) Can we remove "assertJobState(job, JobStateInternal.RUNNING)" in TestJobImpl.testRunningMapperPreemptionWhenReducerIsFinished() since it is not doing anything, and add a comment before the "if(killMappers)" statement saying that the stubbed job cannot finish and we therefore verify task kill events instead?

          4) The description of mapreduce.job.finish-when-all-reducers-done in mapred-default.xml stills says terminate running map tasks. I think we should say something like
          'Specifies whether the job should complete once all reducers have finished, regardless of whether there are still running mappers', which is closer to what really matters to end users. Related to this, we can rename testRunningMappersPreemptedWhenReducerIsFinished and testRunningMappersNotPreemptedWhenReducerIsFinished to something like 'testJobCompletedWhenAllReducersAreFinished' , 'testJobNotCompletedWhenAllReducersAreFinished'.

          Show
          haibochen Haibo Chen added a comment - A few knits: 1) Use block comment /* **/ for checkReadyForCompletionWhenAllReducersDone() 2) We can avoid iterating over all map tasks if job. completingJob is true, that is, if (totalReduces > 0 && totalReduces == completedReduces) { if (!job.completingJob) { for (task: mapTasks) { kill if task is not finished. } job.completingJob = true ; } } 3) Can we remove "assertJobState(job, JobStateInternal.RUNNING)" in TestJobImpl.testRunningMapperPreemptionWhenReducerIsFinished() since it is not doing anything, and add a comment before the "if(killMappers)" statement saying that the stubbed job cannot finish and we therefore verify task kill events instead? 4) The description of mapreduce.job.finish-when-all-reducers-done in mapred-default.xml stills says terminate running map tasks. I think we should say something like 'Specifies whether the job should complete once all reducers have finished, regardless of whether there are still running mappers', which is closer to what really matters to end users. Related to this, we can rename testRunningMappersPreemptedWhenReducerIsFinished and testRunningMappersNotPreemptedWhenReducerIsFinished to something like 'testJobCompletedWhenAllReducersAreFinished' , 'testJobNotCompletedWhenAllReducersAreFinished'.
          Hide
          haibochen Haibo Chen added a comment -

          The checkstyle issues are also relevant, please take a look and fix them as well.

          Show
          haibochen Haibo Chen added a comment - The checkstyle issues are also relevant, please take a look and fix them as well.
          Hide
          hadoopqa Hadoop QA added a comment -
          -1 overall



          Vote Subsystem Runtime Comment
          0 reexec 0m 24s Docker mode activated.
                Prechecks
          +1 @author 0m 0s The patch does not contain any @author tags.
          +1 test4tests 0m 0s The patch appears to include 1 new or modified test files.
                trunk Compile Tests
          0 mvndep 0m 51s Maven dependency ordering for branch
          +1 mvninstall 15m 34s trunk passed
          +1 compile 1m 57s trunk passed
          +1 checkstyle 0m 40s trunk passed
          +1 mvnsite 0m 58s trunk passed
          +1 findbugs 1m 33s trunk passed
          +1 javadoc 0m 42s trunk passed
                Patch Compile Tests
          0 mvndep 0m 8s Maven dependency ordering for patch
          +1 mvninstall 0m 50s the patch passed
          +1 compile 1m 47s the patch passed
          +1 javac 1m 47s the patch passed
          -1 checkstyle 0m 37s hadoop-mapreduce-project/hadoop-mapreduce-client: The patch generated 4 new + 654 unchanged - 5 fixed = 658 total (was 659)
          +1 mvnsite 0m 53s the patch passed
          +1 whitespace 0m 0s The patch has no whitespace issues.
          +1 xml 0m 1s The patch has no ill-formed XML file.
          +1 findbugs 1m 44s the patch passed
          +1 javadoc 0m 38s the patch passed
                Other Tests
          +1 unit 2m 45s hadoop-mapreduce-client-core in the patch passed.
          +1 unit 9m 4s hadoop-mapreduce-client-app in the patch passed.
          +1 asflicense 0m 17s The patch does not generate ASF License warnings.
          42m 13s



          Subsystem Report/Notes
          Docker Image:yetus/hadoop:14b5c93
          JIRA Issue MAPREDUCE-6870
          JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12881011/MAPREDUCE-6870-005.patch
          Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle xml
          uname Linux 435384fda00b 3.13.0-117-generic #164-Ubuntu SMP Fri Apr 7 11:05:26 UTC 2017 x86_64 x86_64 x86_64 GNU/Linux
          Build tool maven
          Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh
          git revision trunk / 8a4bff0
          Default Java 1.8.0_131
          findbugs v3.1.0-RC1
          checkstyle https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/7050/artifact/patchprocess/diff-checkstyle-hadoop-mapreduce-project_hadoop-mapreduce-client.txt
          Test Results https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/7050/testReport/
          modules C: hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-core hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-app U: hadoop-mapreduce-project/hadoop-mapreduce-client
          Console output https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/7050/console
          Powered by Apache Yetus 0.5.0 http://yetus.apache.org

          This message was automatically generated.

          Show
          hadoopqa Hadoop QA added a comment - -1 overall Vote Subsystem Runtime Comment 0 reexec 0m 24s Docker mode activated.       Prechecks +1 @author 0m 0s The patch does not contain any @author tags. +1 test4tests 0m 0s The patch appears to include 1 new or modified test files.       trunk Compile Tests 0 mvndep 0m 51s Maven dependency ordering for branch +1 mvninstall 15m 34s trunk passed +1 compile 1m 57s trunk passed +1 checkstyle 0m 40s trunk passed +1 mvnsite 0m 58s trunk passed +1 findbugs 1m 33s trunk passed +1 javadoc 0m 42s trunk passed       Patch Compile Tests 0 mvndep 0m 8s Maven dependency ordering for patch +1 mvninstall 0m 50s the patch passed +1 compile 1m 47s the patch passed +1 javac 1m 47s the patch passed -1 checkstyle 0m 37s hadoop-mapreduce-project/hadoop-mapreduce-client: The patch generated 4 new + 654 unchanged - 5 fixed = 658 total (was 659) +1 mvnsite 0m 53s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. +1 xml 0m 1s The patch has no ill-formed XML file. +1 findbugs 1m 44s the patch passed +1 javadoc 0m 38s the patch passed       Other Tests +1 unit 2m 45s hadoop-mapreduce-client-core in the patch passed. +1 unit 9m 4s hadoop-mapreduce-client-app in the patch passed. +1 asflicense 0m 17s The patch does not generate ASF License warnings. 42m 13s Subsystem Report/Notes Docker Image:yetus/hadoop:14b5c93 JIRA Issue MAPREDUCE-6870 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12881011/MAPREDUCE-6870-005.patch Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle xml uname Linux 435384fda00b 3.13.0-117-generic #164-Ubuntu SMP Fri Apr 7 11:05:26 UTC 2017 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh git revision trunk / 8a4bff0 Default Java 1.8.0_131 findbugs v3.1.0-RC1 checkstyle https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/7050/artifact/patchprocess/diff-checkstyle-hadoop-mapreduce-project_hadoop-mapreduce-client.txt Test Results https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/7050/testReport/ modules C: hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-core hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-app U: hadoop-mapreduce-project/hadoop-mapreduce-client Console output https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/7050/console Powered by Apache Yetus 0.5.0 http://yetus.apache.org This message was automatically generated.
          Hide
          pbacsko Peter Bacsko added a comment -

          Uploaded patch v6 to address all checkstyle problems.

          Show
          pbacsko Peter Bacsko added a comment - Uploaded patch v6 to address all checkstyle problems.
          Hide
          hadoopqa Hadoop QA added a comment -
          +1 overall



          Vote Subsystem Runtime Comment
          0 reexec 0m 18s Docker mode activated.
                Prechecks
          +1 @author 0m 0s The patch does not contain any @author tags.
          +1 test4tests 0m 0s The patch appears to include 1 new or modified test files.
                trunk Compile Tests
          0 mvndep 0m 8s Maven dependency ordering for branch
          +1 mvninstall 14m 58s trunk passed
          +1 compile 2m 6s trunk passed
          +1 checkstyle 0m 37s trunk passed
          +1 mvnsite 0m 57s trunk passed
          +1 findbugs 1m 30s trunk passed
          +1 javadoc 0m 41s trunk passed
                Patch Compile Tests
          0 mvndep 0m 8s Maven dependency ordering for patch
          +1 mvninstall 0m 51s the patch passed
          +1 compile 2m 0s the patch passed
          +1 javac 2m 0s the patch passed
          +1 checkstyle 0m 36s hadoop-mapreduce-project/hadoop-mapreduce-client: The patch generated 0 new + 654 unchanged - 5 fixed = 654 total (was 659)
          +1 mvnsite 0m 56s the patch passed
          +1 whitespace 0m 0s The patch has no whitespace issues.
          +1 xml 0m 2s The patch has no ill-formed XML file.
          +1 findbugs 1m 46s the patch passed
          +1 javadoc 0m 36s the patch passed
                Other Tests
          +1 unit 2m 48s hadoop-mapreduce-client-core in the patch passed.
          +1 unit 9m 11s hadoop-mapreduce-client-app in the patch passed.
          +1 asflicense 0m 14s The patch does not generate ASF License warnings.
          41m 14s



          Subsystem Report/Notes
          Docker Image:yetus/hadoop:14b5c93
          JIRA Issue MAPREDUCE-6870
          JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12881019/MAPREDUCE-6870-006.patch
          Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle xml
          uname Linux 9b49748d9d9d 3.13.0-117-generic #164-Ubuntu SMP Fri Apr 7 11:05:26 UTC 2017 x86_64 x86_64 x86_64 GNU/Linux
          Build tool maven
          Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh
          git revision trunk / 1a18d5e
          Default Java 1.8.0_131
          findbugs v3.1.0-RC1
          Test Results https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/7051/testReport/
          modules C: hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-core hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-app U: hadoop-mapreduce-project/hadoop-mapreduce-client
          Console output https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/7051/console
          Powered by Apache Yetus 0.5.0 http://yetus.apache.org

          This message was automatically generated.

          Show
          hadoopqa Hadoop QA added a comment - +1 overall Vote Subsystem Runtime Comment 0 reexec 0m 18s Docker mode activated.       Prechecks +1 @author 0m 0s The patch does not contain any @author tags. +1 test4tests 0m 0s The patch appears to include 1 new or modified test files.       trunk Compile Tests 0 mvndep 0m 8s Maven dependency ordering for branch +1 mvninstall 14m 58s trunk passed +1 compile 2m 6s trunk passed +1 checkstyle 0m 37s trunk passed +1 mvnsite 0m 57s trunk passed +1 findbugs 1m 30s trunk passed +1 javadoc 0m 41s trunk passed       Patch Compile Tests 0 mvndep 0m 8s Maven dependency ordering for patch +1 mvninstall 0m 51s the patch passed +1 compile 2m 0s the patch passed +1 javac 2m 0s the patch passed +1 checkstyle 0m 36s hadoop-mapreduce-project/hadoop-mapreduce-client: The patch generated 0 new + 654 unchanged - 5 fixed = 654 total (was 659) +1 mvnsite 0m 56s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. +1 xml 0m 2s The patch has no ill-formed XML file. +1 findbugs 1m 46s the patch passed +1 javadoc 0m 36s the patch passed       Other Tests +1 unit 2m 48s hadoop-mapreduce-client-core in the patch passed. +1 unit 9m 11s hadoop-mapreduce-client-app in the patch passed. +1 asflicense 0m 14s The patch does not generate ASF License warnings. 41m 14s Subsystem Report/Notes Docker Image:yetus/hadoop:14b5c93 JIRA Issue MAPREDUCE-6870 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12881019/MAPREDUCE-6870-006.patch Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle xml uname Linux 9b49748d9d9d 3.13.0-117-generic #164-Ubuntu SMP Fri Apr 7 11:05:26 UTC 2017 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh git revision trunk / 1a18d5e Default Java 1.8.0_131 findbugs v3.1.0-RC1 Test Results https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/7051/testReport/ modules C: hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-core hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-app U: hadoop-mapreduce-project/hadoop-mapreduce-client Console output https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/7051/console Powered by Apache Yetus 0.5.0 http://yetus.apache.org This message was automatically generated.
          Hide
          haibochen Haibo Chen added a comment -

          I think you have the two unit test method names the other way around, that is, job should complete when killMappers is true. Other than that, the patch LGTM.

          Show
          haibochen Haibo Chen added a comment - I think you have the two unit test method names the other way around, that is, job should complete when killMappers is true. Other than that, the patch LGTM.
          Hide
          pbacsko Peter Bacsko added a comment -

          Yep, good catch. Uploading v7.

          Show
          pbacsko Peter Bacsko added a comment - Yep, good catch. Uploading v7.
          Hide
          hadoopqa Hadoop QA added a comment -
          +1 overall



          Vote Subsystem Runtime Comment
          0 reexec 0m 19s Docker mode activated.
                Prechecks
          +1 @author 0m 0s The patch does not contain any @author tags.
          +1 test4tests 0m 0s The patch appears to include 1 new or modified test files.
                trunk Compile Tests
          0 mvndep 0m 7s Maven dependency ordering for branch
          +1 mvninstall 13m 54s trunk passed
          +1 compile 1m 42s trunk passed
          +1 checkstyle 0m 38s trunk passed
          +1 mvnsite 0m 56s trunk passed
          +1 findbugs 1m 26s trunk passed
          +1 javadoc 0m 37s trunk passed
                Patch Compile Tests
          0 mvndep 0m 7s Maven dependency ordering for patch
          +1 mvninstall 0m 47s the patch passed
          +1 compile 1m 38s the patch passed
          +1 javac 1m 38s the patch passed
          +1 checkstyle 0m 36s hadoop-mapreduce-project/hadoop-mapreduce-client: The patch generated 0 new + 654 unchanged - 5 fixed = 654 total (was 659)
          +1 mvnsite 0m 49s the patch passed
          +1 whitespace 0m 0s The patch has no whitespace issues.
          +1 xml 0m 1s The patch has no ill-formed XML file.
          +1 findbugs 1m 35s the patch passed
          +1 javadoc 0m 32s the patch passed
                Other Tests
          +1 unit 2m 39s hadoop-mapreduce-client-core in the patch passed.
          +1 unit 8m 52s hadoop-mapreduce-client-app in the patch passed.
          +1 asflicense 0m 14s The patch does not generate ASF License warnings.
          38m 24s



          Subsystem Report/Notes
          Docker Image:yetus/hadoop:14b5c93
          JIRA Issue MAPREDUCE-6870
          JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12881202/MAPREDUCE-6870-007.patch
          Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle xml
          uname Linux 73c1dc9f7ec6 3.13.0-117-generic #164-Ubuntu SMP Fri Apr 7 11:05:26 UTC 2017 x86_64 x86_64 x86_64 GNU/Linux
          Build tool maven
          Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh
          git revision trunk / 8d953c2
          Default Java 1.8.0_131
          findbugs v3.1.0-RC1
          Test Results https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/7054/testReport/
          modules C: hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-core hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-app U: hadoop-mapreduce-project/hadoop-mapreduce-client
          Console output https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/7054/console
          Powered by Apache Yetus 0.5.0 http://yetus.apache.org

          This message was automatically generated.

          Show
          hadoopqa Hadoop QA added a comment - +1 overall Vote Subsystem Runtime Comment 0 reexec 0m 19s Docker mode activated.       Prechecks +1 @author 0m 0s The patch does not contain any @author tags. +1 test4tests 0m 0s The patch appears to include 1 new or modified test files.       trunk Compile Tests 0 mvndep 0m 7s Maven dependency ordering for branch +1 mvninstall 13m 54s trunk passed +1 compile 1m 42s trunk passed +1 checkstyle 0m 38s trunk passed +1 mvnsite 0m 56s trunk passed +1 findbugs 1m 26s trunk passed +1 javadoc 0m 37s trunk passed       Patch Compile Tests 0 mvndep 0m 7s Maven dependency ordering for patch +1 mvninstall 0m 47s the patch passed +1 compile 1m 38s the patch passed +1 javac 1m 38s the patch passed +1 checkstyle 0m 36s hadoop-mapreduce-project/hadoop-mapreduce-client: The patch generated 0 new + 654 unchanged - 5 fixed = 654 total (was 659) +1 mvnsite 0m 49s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. +1 xml 0m 1s The patch has no ill-formed XML file. +1 findbugs 1m 35s the patch passed +1 javadoc 0m 32s the patch passed       Other Tests +1 unit 2m 39s hadoop-mapreduce-client-core in the patch passed. +1 unit 8m 52s hadoop-mapreduce-client-app in the patch passed. +1 asflicense 0m 14s The patch does not generate ASF License warnings. 38m 24s Subsystem Report/Notes Docker Image:yetus/hadoop:14b5c93 JIRA Issue MAPREDUCE-6870 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12881202/MAPREDUCE-6870-007.patch Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle xml uname Linux 73c1dc9f7ec6 3.13.0-117-generic #164-Ubuntu SMP Fri Apr 7 11:05:26 UTC 2017 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh git revision trunk / 8d953c2 Default Java 1.8.0_131 findbugs v3.1.0-RC1 Test Results https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/7054/testReport/ modules C: hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-core hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-app U: hadoop-mapreduce-project/hadoop-mapreduce-client Console output https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/7054/console Powered by Apache Yetus 0.5.0 http://yetus.apache.org This message was automatically generated.
          Hide
          haibochen Haibo Chen added a comment -

          +1 Will check it in later today.

          Show
          haibochen Haibo Chen added a comment - +1 Will check it in later today.
          Hide
          haibochen Haibo Chen added a comment -

          Thanks Peter Bacsko for the patch! I have committed it to trunk.

          Show
          haibochen Haibo Chen added a comment - Thanks Peter Bacsko for the patch! I have committed it to trunk.
          Hide
          hudson Hudson added a comment -

          SUCCESS: Integrated in Jenkins build Hadoop-trunk-Commit #12162 (See https://builds.apache.org/job/Hadoop-trunk-Commit/12162/)
          MAPREDUCE-6870. Add configuration for MR job to finish when all reducers (haibochen: rev a32e0138fb63c92902e6613001f38a87c8a41321)

          • (edit) hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-app/src/test/java/org/apache/hadoop/mapreduce/v2/app/job/impl/TestJobImpl.java
          • (edit) hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-core/src/main/java/org/apache/hadoop/mapreduce/MRJobConfig.java
          • (edit) hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-core/src/main/resources/mapred-default.xml
          • (edit) hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-app/src/main/java/org/apache/hadoop/mapreduce/v2/app/job/impl/JobImpl.java
          Show
          hudson Hudson added a comment - SUCCESS: Integrated in Jenkins build Hadoop-trunk-Commit #12162 (See https://builds.apache.org/job/Hadoop-trunk-Commit/12162/ ) MAPREDUCE-6870 . Add configuration for MR job to finish when all reducers (haibochen: rev a32e0138fb63c92902e6613001f38a87c8a41321) (edit) hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-app/src/test/java/org/apache/hadoop/mapreduce/v2/app/job/impl/TestJobImpl.java (edit) hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-core/src/main/java/org/apache/hadoop/mapreduce/MRJobConfig.java (edit) hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-core/src/main/resources/mapred-default.xml (edit) hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-app/src/main/java/org/apache/hadoop/mapreduce/v2/app/job/impl/JobImpl.java
          Hide
          xkrogen Erik Krogen added a comment -

          Haibo Chen, Peter Bacsko, thank you for working on this! To provide some context, the reason we wanted it to be configurable is in case mapper tasks have side effects which are expected to be executed in full. For example, you may have a map task which deletes an output directory as it starts, then populates that directory. With this patch in effect, you could potentially wipe the output of a previous map tasks's execution and then never fully repopulate it (since the mapper is preempted). It's a pretty niche case but who knows what MR behavior people might be relying on.

          Given that this patch is enabling the new behavior by default, should this be marked as an incompatible change? Ping Daniel Templeton, Andrew Wang, who I know are working on compatibility guidelines.

          Show
          xkrogen Erik Krogen added a comment - Haibo Chen , Peter Bacsko , thank you for working on this! To provide some context, the reason we wanted it to be configurable is in case mapper tasks have side effects which are expected to be executed in full. For example, you may have a map task which deletes an output directory as it starts, then populates that directory. With this patch in effect, you could potentially wipe the output of a previous map tasks's execution and then never fully repopulate it (since the mapper is preempted). It's a pretty niche case but who knows what MR behavior people might be relying on. Given that this patch is enabling the new behavior by default, should this be marked as an incompatible change? Ping Daniel Templeton , Andrew Wang , who I know are working on compatibility guidelines.
          Hide
          haibochen Haibo Chen added a comment -

          Thanks Erik Krogen for pointing out the incompatibility! I agree. Maybe we even need to flip the default to false in order to avoid the incompatible, since the case this is trying to fix is an edge case to begin with.

          Show
          haibochen Haibo Chen added a comment - Thanks Erik Krogen for pointing out the incompatibility! I agree. Maybe we even need to flip the default to false in order to avoid the incompatible, since the case this is trying to fix is an edge case to begin with.
          Hide
          xkrogen Erik Krogen added a comment - - edited

          Given its rarity and that the worst case scenario is (expected execution time) + (single mapper execution time) I would consider it not a severe issue, which leans me towards compatibility. However the current behavior is pretty confusing for an average user, so, tough call.

          We would like to backport this to older release lines, in which case we definitely need to maintain compatibility and thus have default = false. As for trunk/3.0.0 I am on the fence.

          Show
          xkrogen Erik Krogen added a comment - - edited Given its rarity and that the worst case scenario is (expected execution time) + (single mapper execution time) I would consider it not a severe issue, which leans me towards compatibility. However the current behavior is pretty confusing for an average user, so, tough call. We would like to backport this to older release lines, in which case we definitely need to maintain compatibility and thus have default = false. As for trunk/3.0.0 I am on the fence.
          Hide
          haibochen Haibo Chen added a comment -

          In some cases, the single mapper can take hours to finish, thus delaying job completion by hours. We definitely want to default to false in 2.x for compatibility. For trunk, I think it is a good opportunity to fix it as an incompatible change, unless folks think strongly otherwise. IMO, it's better to fail the niche case in order to not confuse average users.

          Show
          haibochen Haibo Chen added a comment - In some cases, the single mapper can take hours to finish, thus delaying job completion by hours. We definitely want to default to false in 2.x for compatibility. For trunk, I think it is a good opportunity to fix it as an incompatible change, unless folks think strongly otherwise. IMO, it's better to fail the niche case in order to not confuse average users.
          Hide
          xkrogen Erik Krogen added a comment -

          Sounds good; I am in agreement. Since this should be marked as incompatible but the backport should not, shall I create a separate JIRA for the backport, so that we can have the release scripts properly track incompatibility?

          Show
          xkrogen Erik Krogen added a comment - Sounds good; I am in agreement. Since this should be marked as incompatible but the backport should not, shall I create a separate JIRA for the backport, so that we can have the release scripts properly track incompatibility?
          Hide
          haibochen Haibo Chen added a comment -

          Agreed.

          Show
          haibochen Haibo Chen added a comment - Agreed.
          Hide
          templedf Daniel Templeton added a comment -

          Good question. The change breaks the current behavior, but only in unusual cases, and there is nothing that explicitly states the contract is that a job will remain running until all the mappers complete. In fact, with speculative execution, it's just the opposite. What you're breaking here is tradition as opposed to an execution contract, so I would say that it's not meaningfully incompatible. However, the contract around applications is that we don't break anything ever if at all possible. So, strictly speaking, this change is incompatible, and we should mark it as such. To go into branch-2, it will have to be off by default.

          Show
          templedf Daniel Templeton added a comment - Good question. The change breaks the current behavior, but only in unusual cases, and there is nothing that explicitly states the contract is that a job will remain running until all the mappers complete. In fact, with speculative execution, it's just the opposite. What you're breaking here is tradition as opposed to an execution contract, so I would say that it's not meaningfully incompatible. However, the contract around applications is that we don't break anything ever if at all possible. So, strictly speaking, this change is incompatible, and we should mark it as such. To go into branch-2, it will have to be off by default.
          Hide
          andrew.wang Andrew Wang added a comment -

          Hi Erik, do you mind adding a release note summarizing the incompatibility? Would be nice for our end users.

          Show
          andrew.wang Andrew Wang added a comment - Hi Erik, do you mind adding a release note summarizing the incompatibility? Would be nice for our end users.
          Hide
          xkrogen Erik Krogen added a comment -

          Good idea, Andrew Wang, thanks for the reminder. Done.

          Show
          xkrogen Erik Krogen added a comment - Good idea, Andrew Wang , thanks for the reminder. Done.

            People

            • Assignee:
              pbacsko Peter Bacsko
              Reporter:
              zhz Zhe Zhang
            • Votes:
              0 Vote for this issue
              Watchers:
              12 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development