Hadoop QA added a comment - 13/Dec/16 01:49 -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 doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch. 0 mvndep 0m 49s Maven dependency ordering for branch +1 mvninstall 7m 7s trunk passed +1 compile 3m 16s trunk passed +1 checkstyle 1m 31s trunk passed +1 mvnsite 2m 57s trunk passed +1 mvneclipse 1m 52s trunk passed +1 findbugs 3m 50s trunk passed +1 javadoc 2m 14s trunk passed 0 mvndep 0m 29s Maven dependency ordering for patch -1 mvninstall 0m 21s hadoop-mapreduce-client-core in the patch failed. -1 compile 0m 18s hadoop-mapreduce-project in the patch failed. -1 javac 0m 18s hadoop-mapreduce-project in the patch failed. +1 checkstyle 0m 29s hadoop-mapreduce-project: The patch generated 0 new + 318 unchanged - 18 fixed = 318 total (was 336) -1 mvnsite 0m 17s hadoop-mapreduce-client-core in the patch failed. +1 mvneclipse 0m 26s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. -1 findbugs 0m 16s hadoop-mapreduce-client-core in the patch failed. +1 javadoc 0m 32s the patch passed -1 unit 0m 17s hadoop-mapreduce-client-core in the patch failed. +1 unit 0m 31s hadoop-mapreduce-examples in the patch passed. +1 asflicense 0m 15s The patch does not generate ASF License warnings. 31m 3s Subsystem Report/Notes Docker Image:yetus/hadoop:a9ad5d6 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12842908/MAPREDUCE-6715.001.patch JIRA Issue MAPREDUCE-6715 Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle uname Linux 4d0396b9bc35 3.13.0-95-generic #142-Ubuntu SMP Fri Aug 12 17:00:09 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh git revision trunk / c6a3923 Default Java 1.8.0_111 findbugs v3.0.0 mvninstall https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/6842/artifact/patchprocess/patch-mvninstall-hadoop-mapreduce-project_hadoop-mapreduce-client_hadoop-mapreduce-client-core.txt compile https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/6842/artifact/patchprocess/patch-compile-hadoop-mapreduce-project.txt javac https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/6842/artifact/patchprocess/patch-compile-hadoop-mapreduce-project.txt mvnsite https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/6842/artifact/patchprocess/patch-mvnsite-hadoop-mapreduce-project_hadoop-mapreduce-client_hadoop-mapreduce-client-core.txt findbugs https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/6842/artifact/patchprocess/patch-findbugs-hadoop-mapreduce-project_hadoop-mapreduce-client_hadoop-mapreduce-client-core.txt unit https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/6842/artifact/patchprocess/patch-unit-hadoop-mapreduce-project_hadoop-mapreduce-client_hadoop-mapreduce-client-core.txt Test Results https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/6842/testReport/ modules C: hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-core hadoop-mapreduce-project/hadoop-mapreduce-examples U: hadoop-mapreduce-project Console output https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/6842/console Powered by Apache Yetus 0.3.0 http://yetus.apache.org This message was automatically generated.

Hadoop QA added a comment - 14/Dec/16 01:49 -1 overall Vote Subsystem Runtime Comment 0 reexec 0m 16s Docker mode activated. +1 @author 0m 0s The patch does not contain any @author tags. -1 test4tests 0m 0s The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch. 0 mvndep 0m 52s Maven dependency ordering for branch +1 mvninstall 7m 9s trunk passed +1 compile 2m 12s trunk passed +1 checkstyle 0m 32s trunk passed +1 mvnsite 0m 58s trunk passed +1 mvneclipse 0m 31s trunk passed +1 findbugs 1m 37s trunk passed +1 javadoc 0m 39s trunk passed 0 mvndep 0m 10s Maven dependency ordering for patch +1 mvninstall 0m 45s the patch passed +1 compile 2m 6s the patch passed +1 javac 2m 6s the patch passed +1 checkstyle 0m 34s hadoop-mapreduce-project: The patch generated 0 new + 318 unchanged - 18 fixed = 318 total (was 336) +1 mvnsite 0m 57s the patch passed +1 mvneclipse 0m 32s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. +1 findbugs 1m 39s the patch passed +1 javadoc 0m 32s the patch passed +1 unit 2m 47s hadoop-mapreduce-client-core in the patch passed. +1 unit 0m 31s hadoop-mapreduce-examples in the patch passed. +1 asflicense 0m 17s The patch does not generate ASF License warnings. 26m 25s Subsystem Report/Notes Docker Image:yetus/hadoop:a9ad5d6 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12843129/MAPREDUCE-6715.002.patch JIRA Issue MAPREDUCE-6715 Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle uname Linux 649e9383213c 3.13.0-95-generic #142-Ubuntu SMP Fri Aug 12 17:00:09 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh git revision trunk / e24a923 Default Java 1.8.0_111 findbugs v3.0.0 Test Results https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/6844/testReport/ modules C: hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-core hadoop-mapreduce-project/hadoop-mapreduce-examples U: hadoop-mapreduce-project Console output https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/6844/console Powered by Apache Yetus 0.3.0 http://yetus.apache.org This message was automatically generated.

Daniel Templeton added a comment - 28/Dec/16 23:18 Thanks, Yufei Gu . Some comments: In CleanupQueue , you're missing a space in the text at the line break In ShuffleSchedulerImpl , the LOG.debug() should be in a conditional In MapTask if you return null , you're going to cause NPEs

Yufei Gu added a comment - 29/Dec/16 01:39 Thanks Daniel Templeton for the review. Uploaded a new patch for all your comments.

Hadoop QA added a comment - 29/Dec/16 02:23 -1 overall Vote Subsystem Runtime Comment 0 reexec 0m 12s Docker mode activated. +1 @author 0m 0s The patch does not contain any @author tags. -1 test4tests 0m 0s The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch. 0 mvndep 0m 9s Maven dependency ordering for branch +1 mvninstall 13m 40s trunk passed +1 compile 2m 7s trunk passed +1 checkstyle 0m 41s trunk passed +1 mvnsite 0m 59s trunk passed +1 mvneclipse 0m 40s trunk passed +1 findbugs 1m 36s trunk passed +1 javadoc 0m 49s trunk passed 0 mvndep 0m 11s Maven dependency ordering for patch +1 mvninstall 0m 55s the patch passed +1 compile 2m 13s the patch passed +1 javac 2m 13s the patch passed +1 checkstyle 0m 39s hadoop-mapreduce-project: The patch generated 0 new + 318 unchanged - 18 fixed = 318 total (was 336) +1 mvnsite 0m 59s the patch passed +1 mvneclipse 0m 35s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. +1 findbugs 1m 50s the patch passed +1 javadoc 0m 46s the patch passed +1 unit 3m 25s hadoop-mapreduce-client-core in the patch passed. +1 unit 0m 37s hadoop-mapreduce-examples in the patch passed. +1 asflicense 0m 33s The patch does not generate ASF License warnings. 34m 35s Subsystem Report/Notes Docker Image:yetus/hadoop:a9ad5d6 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12845004/MAPREDUCE-6715.003.patch JIRA Issue MAPREDUCE-6715 Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle uname Linux 62719fe18f1e 3.13.0-106-generic #153-Ubuntu SMP Tue Dec 6 15:44:32 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh git revision trunk / f216276 Default Java 1.8.0_111 findbugs v3.0.0 Test Results https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/6852/testReport/ modules C: hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-core hadoop-mapreduce-project/hadoop-mapreduce-examples U: hadoop-mapreduce-project Console output https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/6852/console Powered by Apache Yetus 0.3.0 http://yetus.apache.org This message was automatically generated.

Daniel Templeton added a comment - 03/Jan/17 19:36 Thanks for the updates. Just noticed a couple other things: lastException.getMessage() is often empty, so lastException.toString() is better ReflectionUtils.newInstance() cannot return null, so the added null check is pointless "Error in last collector was :" should be "Error in last collector was: " String line = in.readLine(); while (line != null ) { result.add(line); line = in.readLine(); } can be simplified to: String line; while ((line = in.readLine()) != null ) { result.add(line); } Your call if that's actually simpler

Yufei Gu added a comment - 03/Jan/17 20:05 Thanks Daniel Templeton for the review. I uploaded patch 004 for your comments. String line = in.readLine(); while (line != null ) { result.add(line); line = in.readLine(); } sounds simple enough to me since there are only two line in the loop and to remove one line does make it a little bit simpler but the while condition clause gets more complex though.

Hadoop QA added a comment - 03/Jan/17 20:39 -1 overall Vote Subsystem Runtime Comment 0 reexec 0m 12s Docker mode activated. +1 @author 0m 0s The patch does not contain any @author tags. -1 test4tests 0m 0s The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch. 0 mvndep 0m 55s Maven dependency ordering for branch +1 mvninstall 13m 2s trunk passed +1 compile 1m 52s trunk passed +1 checkstyle 0m 36s trunk passed +1 mvnsite 0m 51s trunk passed +1 mvneclipse 0m 30s trunk passed +1 findbugs 1m 21s trunk passed +1 javadoc 0m 38s trunk passed 0 mvndep 0m 9s Maven dependency ordering for patch +1 mvninstall 0m 40s the patch passed +1 compile 1m 43s the patch passed +1 javac 1m 43s the patch passed +1 checkstyle 0m 30s hadoop-mapreduce-project: The patch generated 0 new + 308 unchanged - 18 fixed = 308 total (was 326) +1 mvnsite 0m 45s the patch passed +1 mvneclipse 0m 25s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. +1 findbugs 1m 32s the patch passed +1 javadoc 0m 34s the patch passed +1 unit 2m 52s hadoop-mapreduce-client-core in the patch passed. +1 unit 0m 31s hadoop-mapreduce-examples in the patch passed. +1 asflicense 0m 23s The patch does not generate ASF License warnings. 30m 51s Subsystem Report/Notes Docker Image:yetus/hadoop:a9ad5d6 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12845425/MAPREDUCE-6715.004.patch JIRA Issue MAPREDUCE-6715 Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle uname Linux 4e2d83d413db 3.13.0-106-generic #153-Ubuntu SMP Tue Dec 6 15:44:32 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh git revision trunk / 7fcc73f Default Java 1.8.0_111 findbugs v3.0.0 Test Results https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/6853/testReport/ modules C: hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-core hadoop-mapreduce-project/hadoop-mapreduce-examples U: hadoop-mapreduce-project Console output https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/6853/console Powered by Apache Yetus 0.3.0 http://yetus.apache.org This message was automatically generated.

Daniel Templeton added a comment - 05/Jan/17 15:31 LGTM. About that pointless null check... If it's there to make a code analyzer happy, I'm OK with that. It might be good to add a comment to explain that, though, so that someone else doesn't remove it later.

Yufei Gu added a comment - 05/Jan/17 22:23 Thanks for the review. After deeper looking, I realize this complain about NPE is because the code analyzer cannot tell whether two separate code section are in the same condition. For example, section1 and section2 in the following code are under the same condition, but code analyzer cannot tell. I adjusted the code a little, hopefully it will work for the code analyzer, but it is NOT an issue anyway. We may need a smarter code analyzer. CompressionCodec codec = null ; String extension = ""; if (isCompressed) { // section1 Class <? extends CompressionCodec> codecClass = getOutputCompressorClass(job, GzipCodec.class); codec = (CompressionCodec) ReflectionUtils.newInstance(codecClass, conf); extension = codec.getDefaultExtension(); } Path file = getDefaultWorkFile(job, extension); FileSystem fs = file.getFileSystem(conf); if (!isCompressed) { FSDataOutputStream fileOut = fs.create(file, false ); return new LineRecordWriter<K, V>(fileOut, keyValueSeparator); } else { //section2 FSDataOutputStream fileOut = fs.create(file, false ); return new LineRecordWriter<K, V>( new DataOutputStream (codec.createOutputStream(fileOut)), keyValueSeparator); }

Daniel Templeton added a comment - 05/Jan/17 22:39 That part looks fine to me. I'm looking more closely at the ShuffleSchedulerImpl changes now. First, throwing InterruptedException in the wrong thing to do. Second, the analyzer is wrong-- host can't be null. Maybe if you rearrange the code like this: Iterator<MapHost> iter = pendingHosts.iterator(); // Safe to take one because we know pendingHosts isn't empty host = iter.next(); int numToPick = random.nextInt(pendingHosts.size()); for ( int i=0; i < numToPick; ++i) { host = iter.next(); } the analyzer would be happier?

Hadoop QA added a comment - 05/Jan/17 22:51 -1 overall Vote Subsystem Runtime Comment 0 reexec 0m 26s Docker mode activated. +1 @author 0m 0s The patch does not contain any @author tags. -1 test4tests 0m 0s The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch. 0 mvndep 1m 39s Maven dependency ordering for branch +1 mvninstall 15m 18s trunk passed +1 compile 1m 42s trunk passed +1 checkstyle 0m 31s trunk passed +1 mvnsite 0m 46s trunk passed +1 mvneclipse 0m 29s trunk passed +1 findbugs 1m 15s trunk passed +1 javadoc 0m 35s trunk passed 0 mvndep 0m 8s Maven dependency ordering for patch +1 mvninstall 0m 39s the patch passed +1 compile 1m 40s the patch passed +1 javac 1m 40s the patch passed +1 checkstyle 0m 29s hadoop-mapreduce-project: The patch generated 0 new + 315 unchanged - 20 fixed = 315 total (was 335) +1 mvnsite 0m 41s the patch passed +1 mvneclipse 0m 25s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. +1 findbugs 1m 26s the patch passed +1 javadoc 0m 32s the patch passed +1 unit 2m 46s hadoop-mapreduce-client-core in the patch passed. +1 unit 0m 29s hadoop-mapreduce-examples in the patch passed. +1 asflicense 0m 18s The patch does not generate ASF License warnings. 32m 57s Subsystem Report/Notes Docker Image:yetus/hadoop:a9ad5d6 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12845864/MAPREDUCE-6715.005.patch JIRA Issue MAPREDUCE-6715 Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle uname Linux 7f5232d4f5d2 3.13.0-96-generic #143-Ubuntu SMP Mon Aug 29 20:15:20 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh git revision trunk / 02766b6 Default Java 1.8.0_111 findbugs v3.0.0 Test Results https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/6859/testReport/ modules C: hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-core hadoop-mapreduce-project/hadoop-mapreduce-examples U: hadoop-mapreduce-project Console output https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/6859/console Powered by Apache Yetus 0.3.0 http://yetus.apache.org This message was automatically generated.

Yufei Gu added a comment - 05/Jan/17 23:02 Thanks Daniel Templeton , makes sense to me, uploaded a new patch.

Hadoop QA added a comment - 05/Jan/17 23:43 -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 doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch. 0 mvndep 0m 17s Maven dependency ordering for branch +1 mvninstall 16m 32s trunk passed +1 compile 1m 48s trunk passed +1 checkstyle 0m 32s trunk passed +1 mvnsite 0m 48s trunk passed +1 mvneclipse 0m 30s trunk passed +1 findbugs 1m 22s trunk passed +1 javadoc 0m 39s trunk passed 0 mvndep 0m 11s Maven dependency ordering for patch +1 mvninstall 0m 56s the patch passed +1 compile 1m 54s the patch passed +1 javac 1m 54s the patch passed +1 checkstyle 0m 30s hadoop-mapreduce-project: The patch generated 0 new + 316 unchanged - 20 fixed = 316 total (was 336) +1 mvnsite 0m 43s the patch passed +1 mvneclipse 0m 25s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. +1 findbugs 1m 33s the patch passed +1 javadoc 0m 31s the patch passed +1 unit 2m 48s hadoop-mapreduce-client-core in the patch passed. +1 unit 0m 31s hadoop-mapreduce-examples in the patch passed. +1 asflicense 0m 18s The patch does not generate ASF License warnings. 34m 12s Subsystem Report/Notes Docker Image:yetus/hadoop:a9ad5d6 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12845880/MAPREDUCE-6715.006.patch JIRA Issue MAPREDUCE-6715 Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle uname Linux 26a1e7d91916 3.13.0-106-generic #153-Ubuntu SMP Tue Dec 6 15:44:32 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh git revision trunk / 02766b6 Default Java 1.8.0_111 findbugs v3.0.0 Test Results https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/6860/testReport/ modules C: hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-core hadoop-mapreduce-project/hadoop-mapreduce-examples U: hadoop-mapreduce-project Console output https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/6860/console Powered by Apache Yetus 0.3.0 http://yetus.apache.org This message was automatically generated.

Daniel Templeton added a comment - 06/Jan/17 01:01 Yay! LGTM +1 I'll commit shortly.

Daniel Templeton added a comment - 06/Jan/17 02:05 Thanks for the patch, Yufei Gu . Committed to trunk and branch-2

Hudson added a comment - 06/Jan/17 02:37 SUCCESS: Integrated in Jenkins build Hadoop-trunk-Commit #11079 (See https://builds.apache.org/job/Hadoop-trunk-Commit/11079/ ) MAPREDUCE-6715 . Fix Several Unsafe Practices (Contributed by Yufei Gu (templedf: rev 0b8a7c18ddbe73b356b3c9baf4460659ccaee095) (edit) hadoop-mapreduce-project/hadoop-mapreduce-examples/src/main/java/org/apache/hadoop/examples/dancing/Pentomino.java (edit) hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-core/src/main/java/org/apache/hadoop/mapred/MapTask.java (edit) hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-core/src/main/java/org/apache/hadoop/mapreduce/task/reduce/ShuffleSchedulerImpl.java (edit) hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-core/src/main/java/org/apache/hadoop/mapreduce/lib/output/TextOutputFormat.java (edit) hadoop-mapreduce-project/hadoop-mapreduce-examples/src/main/java/org/apache/hadoop/examples/terasort/TeraScheduler.java (edit) hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-core/src/main/java/org/apache/hadoop/mapred/CleanupQueue.java

Yufei Gu added a comment - 06/Jan/17 17:38 Thanks Daniel Templeton for the review and commit.