Uploaded image for project: 'Hadoop Common'
  1. Hadoop Common
  2. HADOOP-13709

Ability to clean up subprocesses spawned by Shell when the process exits

    Details

    • Type: Improvement
    • Status: Resolved
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: 2.2.0
    • Fix Version/s: 2.9.0, 3.0.0-alpha2
    • Component/s: None
    • Labels:
      None
    • Target Version/s:
    • Hadoop Flags:
      Reviewed

      Description

      The runCommand code in Shell.java can get into a situation where it will ignore InterruptedExceptions and refuse to shutdown due to being in I/O waiting for the return value of the subprocess that was spawned. We need to allow for the subprocess to be interrupted and killed when the shell process gets killed. Currently the JVM will shutdown and all of the subprocesses will be orphaned and not killed.

      1. HADOOP-13709.001.patch
        7 kB
        Eric Badger
      2. HADOOP-13709.002.patch
        2 kB
        Eric Badger
      3. HADOOP-13709.003.patch
        4 kB
        Eric Badger
      4. HADOOP-13709.004.patch
        4 kB
        Eric Badger
      5. HADOOP-13709.005.patch
        4 kB
        Eric Badger
      6. HADOOP-13709.006.patch
        4 kB
        Eric Badger
      7. HADOOP-13709.007.patch
        5 kB
        Eric Badger
      8. HADOOP-13709.008.patch
        5 kB
        Eric Badger
      9. HADOOP-13709.009.patch
        5 kB
        Eric Badger
      10. HADOOP-13709.009.patch
        5 kB
        Eric Badger
      11. HADOOP-13709-branch-2.010.patch
        4 kB
        Eric Badger

        Issue Links

          Activity

          Hide
          ebadger Eric Badger added a comment -

          Attaching patch with the hadoop-common portion of the fix. YARN-5641 includes the YARN portion of the fix.

          Show
          ebadger Eric Badger added a comment - Attaching patch with the hadoop-common portion of the fix. YARN-5641 includes the YARN portion of the fix.
          Hide
          hadoopqa Hadoop QA added a comment -
          -1 overall



          Vote Subsystem Runtime Comment
          0 reexec 0m 18s 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.
          +1 mvninstall 7m 49s trunk passed
          +1 compile 8m 9s trunk passed
          +1 checkstyle 0m 24s trunk passed
          +1 mvnsite 1m 4s trunk passed
          +1 mvneclipse 0m 13s trunk passed
          +1 findbugs 1m 39s trunk passed
          +1 javadoc 0m 46s trunk passed
          +1 mvninstall 0m 43s the patch passed
          +1 compile 8m 27s the patch passed
          +1 javac 8m 27s the patch passed
          -0 checkstyle 0m 27s hadoop-common-project/hadoop-common: The patch generated 2 new + 55 unchanged - 0 fixed = 57 total (was 55)
          +1 mvnsite 1m 6s the patch passed
          +1 mvneclipse 0m 14s the patch passed
          +1 whitespace 0m 0s The patch has no whitespace issues.
          +1 findbugs 1m 50s the patch passed
          +1 javadoc 0m 47s the patch passed
          -1 unit 8m 55s hadoop-common in the patch failed.
          +1 asflicense 0m 21s The patch does not generate ASF License warnings.
          44m 42s



          Reason Tests
          Failed junit tests hadoop.ha.TestZKFailoverController
            hadoop.util.TestShell



          Subsystem Report/Notes
          Docker Image:yetus/hadoop:9560f25
          JIRA Issue HADOOP-13709
          JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12832727/HADOOP-13709.001.patch
          Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle
          uname Linux 492759b14e18 3.13.0-36-lowlatency #63-Ubuntu SMP PREEMPT Wed Sep 3 21:56:12 UTC 2014 x86_64 x86_64 x86_64 GNU/Linux
          Build tool maven
          Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh
          git revision trunk / 2fb392a
          Default Java 1.8.0_101
          findbugs v3.0.0
          checkstyle https://builds.apache.org/job/PreCommit-HADOOP-Build/10733/artifact/patchprocess/diff-checkstyle-hadoop-common-project_hadoop-common.txt
          unit https://builds.apache.org/job/PreCommit-HADOOP-Build/10733/artifact/patchprocess/patch-unit-hadoop-common-project_hadoop-common.txt
          Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/10733/testReport/
          modules C: hadoop-common-project/hadoop-common U: hadoop-common-project/hadoop-common
          Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/10733/console
          Powered by Apache Yetus 0.4.0-SNAPSHOT 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. +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. +1 mvninstall 7m 49s trunk passed +1 compile 8m 9s trunk passed +1 checkstyle 0m 24s trunk passed +1 mvnsite 1m 4s trunk passed +1 mvneclipse 0m 13s trunk passed +1 findbugs 1m 39s trunk passed +1 javadoc 0m 46s trunk passed +1 mvninstall 0m 43s the patch passed +1 compile 8m 27s the patch passed +1 javac 8m 27s the patch passed -0 checkstyle 0m 27s hadoop-common-project/hadoop-common: The patch generated 2 new + 55 unchanged - 0 fixed = 57 total (was 55) +1 mvnsite 1m 6s the patch passed +1 mvneclipse 0m 14s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. +1 findbugs 1m 50s the patch passed +1 javadoc 0m 47s the patch passed -1 unit 8m 55s hadoop-common in the patch failed. +1 asflicense 0m 21s The patch does not generate ASF License warnings. 44m 42s Reason Tests Failed junit tests hadoop.ha.TestZKFailoverController   hadoop.util.TestShell Subsystem Report/Notes Docker Image:yetus/hadoop:9560f25 JIRA Issue HADOOP-13709 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12832727/HADOOP-13709.001.patch Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle uname Linux 492759b14e18 3.13.0-36-lowlatency #63-Ubuntu SMP PREEMPT Wed Sep 3 21:56:12 UTC 2014 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh git revision trunk / 2fb392a Default Java 1.8.0_101 findbugs v3.0.0 checkstyle https://builds.apache.org/job/PreCommit-HADOOP-Build/10733/artifact/patchprocess/diff-checkstyle-hadoop-common-project_hadoop-common.txt unit https://builds.apache.org/job/PreCommit-HADOOP-Build/10733/artifact/patchprocess/patch-unit-hadoop-common-project_hadoop-common.txt Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/10733/testReport/ modules C: hadoop-common-project/hadoop-common U: hadoop-common-project/hadoop-common Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/10733/console Powered by Apache Yetus 0.4.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.
          Hide
          ebadger Eric Badger added a comment -

          TestShell fails because it does not have the YARN component of the fix from YARN-5641. I've manually tested that it passes with that fix.

          Show
          ebadger Eric Badger added a comment - TestShell fails because it does not have the YARN component of the fix from YARN-5641 . I've manually tested that it passes with that fix.
          Hide
          andrew.wang Andrew Wang added a comment -

          Hi Eric, do you mind setting affects and target versions for this JIRA?

          Overall the change looks good, though since this seems related to YARN I'll let someone else +1.

          Show
          andrew.wang Andrew Wang added a comment - Hi Eric, do you mind setting affects and target versions for this JIRA? Overall the change looks good, though since this seems related to YARN I'll let someone else +1.
          Hide
          ebadger Eric Badger added a comment - - edited

          Andrew Wang, the code's been this way since Mapreduce was put into separate projects back in 2009. I put down the affect version as 2.2, but it goes back further than that.

          Show
          ebadger Eric Badger added a comment - - edited Andrew Wang , the code's been this way since Mapreduce was put into separate projects back in 2009. I put down the affect version as 2.2, but it goes back further than that.
          Hide
          ebadger Eric Badger added a comment -

          TestShell fails because it does not have the YARN component of the fix from YARN-5641. I've manually tested that it passes with that fix.

          I was incorrect about this. This patch is completely independent of (though required by) YARN-5641. I did manually test that TestShell#testShellCommandTimerLeak passes, but the failure may be related to this patch.

          Show
          ebadger Eric Badger added a comment - TestShell fails because it does not have the YARN component of the fix from YARN-5641 . I've manually tested that it passes with that fix. I was incorrect about this. This patch is completely independent of (though required by) YARN-5641 . I did manually test that TestShell#testShellCommandTimerLeak passes, but the failure may be related to this patch.
          Hide
          ebadger Eric Badger added a comment - - edited

          The TestShell#testShellCommandTimerLeak failure in the precommit seems to be related to this patch. Locally it failed due to timeout 8/500 times with the patch, but 0/500 times without the patch. The failure is happening when the code gets blocked calling inReader.close(). Right above this code there is a comment about how there was a JDK 7 issue that caused a race with trying to close the stream, but that was supposedly fixed by the addition of synchronized (stdout).

          Show
          ebadger Eric Badger added a comment - - edited The TestShell#testShellCommandTimerLeak failure in the precommit seems to be related to this patch. Locally it failed due to timeout 8/500 times with the patch, but 0/500 times without the patch. The failure is happening when the code gets blocked calling inReader.close() . Right above this code there is a comment about how there was a JDK 7 issue that caused a race with trying to close the stream, but that was supposedly fixed by the addition of synchronized (stdout) .
          Hide
          daryn Daryn Sharp added a comment -

          If you catch it in the act, I bet its going to be some kind of deadlock between the jvm's shell reaper thread, the main process, and the new process doing the parseExecResult. There's too many threads accessing highly synchronized readers/streams.

          I'm uneasy about this patch in general. Changing the semantics of anything in hadoop-common is fraught with peril. Subclasses are not expecting their parseExecResult to be run in a thread which may introduce subtle errors including but not limited to synchronization/volatility issues. Or if method uses thread locals but now it's in a different thread, etc.

          While interrupting a thread in a shell command would be nice, I think the more general requirement for yarn is that processes are not left running. It's probably a safer assumption that no hadoop service wants lingering processes. Perhaps create a set of all running shell instances. A shutdown hook could call destroy() on all the instances still registered.

          Show
          daryn Daryn Sharp added a comment - If you catch it in the act, I bet its going to be some kind of deadlock between the jvm's shell reaper thread, the main process, and the new process doing the parseExecResult. There's too many threads accessing highly synchronized readers/streams. I'm uneasy about this patch in general. Changing the semantics of anything in hadoop-common is fraught with peril. Subclasses are not expecting their parseExecResult to be run in a thread which may introduce subtle errors including but not limited to synchronization/volatility issues. Or if method uses thread locals but now it's in a different thread, etc. While interrupting a thread in a shell command would be nice, I think the more general requirement for yarn is that processes are not left running. It's probably a safer assumption that no hadoop service wants lingering processes. Perhaps create a set of all running shell instances. A shutdown hook could call destroy() on all the instances still registered.
          Hide
          ebadger Eric Badger added a comment -

          Daryn Sharp, good catch! I was able to recreate the hung test locally and took a jstack. Looks like it is indeed deadlocking between threads reading the stdout stream.

          I'm not super thrilled about using a shutdown hook to clean up the processes, but in this case I think it will work if we can't think of anything else. Jason Lowe, do you have any additional insights?

          Found one Java-level deadlock:
          =============================
          "pool-4-thread-1":
            waiting to lock monitor 0x00007f5900002d78 (object 0x00000000d67243a0, a java.lang.UNIXProcess$ProcessPipeInputStream),
            which is held by "Thread-0"
          "Thread-0":
            waiting to lock monitor 0x00007f58f8006008 (object 0x00000000d672ec28, a java.io.InputStreamReader),
            which is held by "pool-4-thread-1"
          
          Java stack information for the threads listed above:
          ===================================================
          "pool-4-thread-1":
                  at java.io.BufferedInputStream.read(BufferedInputStream.java:336)
                  - waiting to lock <0x00000000d67243a0> (a java.lang.UNIXProcess$ProcessPipeInputStream)
                  at sun.nio.cs.StreamDecoder.readBytes(StreamDecoder.java:284)
                  at sun.nio.cs.StreamDecoder.implRead(StreamDecoder.java:326)
                  at sun.nio.cs.StreamDecoder.read(StreamDecoder.java:178)
                  - locked <0x00000000d672ec28> (a java.io.InputStreamReader)
                  at java.io.InputStreamReader.read(InputStreamReader.java:184)
                  at java.io.BufferedReader.fill(BufferedReader.java:161)
                  at java.io.BufferedReader.read1(BufferedReader.java:212)
                  at java.io.BufferedReader.read(BufferedReader.java:286)
                  - locked <0x00000000d672ec28> (a java.io.InputStreamReader)
                  at org.apache.hadoop.util.Shell$ShellCommandExecutor.parseExecResult(Shell.java:1214)
                  at org.apache.hadoop.util.Shell$2.call(Shell.java:965)
                  at org.apache.hadoop.util.Shell$2.call(Shell.java:962)
                  at java.util.concurrent.FutureTask.run(FutureTask.java:266)
                  at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1142)
                  at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:617)
                  at java.lang.Thread.run(Thread.java:745)
          "Thread-0":
                  at java.io.BufferedReader.close(BufferedReader.java:522)
                  - waiting to lock <0x00000000d672ec28> (a java.io.InputStreamReader)
                  at org.apache.hadoop.util.Shell.runCommand(Shell.java:1029)
                  - locked <0x00000000d67243a0> (a java.lang.UNIXProcess$ProcessPipeInputStream)
                  at org.apache.hadoop.util.Shell.run(Shell.java:883)
                  at org.apache.hadoop.util.Shell$ShellCommandExecutor.execute(Shell.java:1201)
                  at org.apache.hadoop.util.TestShell.testShellCommandTimerLeak(TestShell.java:241)
                  at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
                  at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
                  at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
                  at java.lang.reflect.Method.invoke(Method.java:497)
                  at org.junit.runners.model.FrameworkMethod$1.runReflectiveCall(FrameworkMethod.java:47)
                  at org.junit.internal.runners.model.ReflectiveCallable.run(ReflectiveCallable.java:12)
                  at org.junit.runners.model.FrameworkMethod.invokeExplosively(FrameworkMethod.java:44)
                  at org.junit.internal.runners.statements.InvokeMethod.evaluate(InvokeMethod.java:17)
                  at org.junit.internal.runners.statements.RunBefores.evaluate(RunBefores.java:26)
                  at org.junit.internal.runners.statements.FailOnTimeout$StatementThread.run(FailOnTimeout.java:74)
          
          Found 1 deadlock.
          
          Show
          ebadger Eric Badger added a comment - Daryn Sharp , good catch! I was able to recreate the hung test locally and took a jstack. Looks like it is indeed deadlocking between threads reading the stdout stream. I'm not super thrilled about using a shutdown hook to clean up the processes, but in this case I think it will work if we can't think of anything else. Jason Lowe , do you have any additional insights? Found one Java-level deadlock: ============================= "pool-4-thread-1":   waiting to lock monitor 0x00007f5900002d78 (object 0x00000000d67243a0, a java.lang.UNIXProcess$ProcessPipeInputStream),   which is held by "Thread-0" "Thread-0":   waiting to lock monitor 0x00007f58f8006008 (object 0x00000000d672ec28, a java.io.InputStreamReader),   which is held by "pool-4-thread-1" Java stack information for the threads listed above: =================================================== "pool-4-thread-1":         at java.io.BufferedInputStream.read(BufferedInputStream.java:336)         - waiting to lock <0x00000000d67243a0> (a java.lang.UNIXProcess$ProcessPipeInputStream)         at sun.nio.cs.StreamDecoder.readBytes(StreamDecoder.java:284)         at sun.nio.cs.StreamDecoder.implRead(StreamDecoder.java:326)         at sun.nio.cs.StreamDecoder.read(StreamDecoder.java:178)         - locked <0x00000000d672ec28> (a java.io.InputStreamReader)         at java.io.InputStreamReader.read(InputStreamReader.java:184)         at java.io.BufferedReader.fill(BufferedReader.java:161)         at java.io.BufferedReader.read1(BufferedReader.java:212)         at java.io.BufferedReader.read(BufferedReader.java:286)         - locked <0x00000000d672ec28> (a java.io.InputStreamReader)         at org.apache.hadoop.util.Shell$ShellCommandExecutor.parseExecResult(Shell.java:1214)         at org.apache.hadoop.util.Shell$2.call(Shell.java:965)         at org.apache.hadoop.util.Shell$2.call(Shell.java:962)         at java.util.concurrent.FutureTask.run(FutureTask.java:266)         at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1142)         at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:617)         at java.lang.Thread.run(Thread.java:745) "Thread-0":         at java.io.BufferedReader.close(BufferedReader.java:522)         - waiting to lock <0x00000000d672ec28> (a java.io.InputStreamReader)         at org.apache.hadoop.util.Shell.runCommand(Shell.java:1029)         - locked <0x00000000d67243a0> (a java.lang.UNIXProcess$ProcessPipeInputStream)         at org.apache.hadoop.util.Shell.run(Shell.java:883)         at org.apache.hadoop.util.Shell$ShellCommandExecutor.execute(Shell.java:1201)         at org.apache.hadoop.util.TestShell.testShellCommandTimerLeak(TestShell.java:241)         at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)         at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)         at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)         at java.lang.reflect.Method.invoke(Method.java:497)         at org.junit.runners.model.FrameworkMethod$1.runReflectiveCall(FrameworkMethod.java:47)         at org.junit.internal.runners.model.ReflectiveCallable.run(ReflectiveCallable.java:12)         at org.junit.runners.model.FrameworkMethod.invokeExplosively(FrameworkMethod.java:44)         at org.junit.internal.runners.statements.InvokeMethod.evaluate(InvokeMethod.java:17)         at org.junit.internal.runners.statements.RunBefores.evaluate(RunBefores.java:26)         at org.junit.internal.runners.statements.FailOnTimeout$StatementThread.run(FailOnTimeout.java:74) Found 1 deadlock.
          Hide
          jlowe Jason Lowe added a comment -

          Good catch, Daryn! I missed the memory-model races introduced by moving the stdout processing to a new thread. We could fix that with appropriate synchronization in the derived classes. However Shell is explicitly marked Public, so it's prudent to avoid disrupting that too much.

          It would be really nice to be able to kill the subprocess if the thread executing Shell is interrupted, but without moving the stdout processing to another thread I don't see a good way to do that. Reading from a stream doesn't seem to be interruptible in practice. If we can't make the Shell command reliably interruptible then the next best thing is to make sure we don't leave them lingering when this process exits so we can fix the problem described in YARN-5641. Having a live Shell list that we can traverse on shutdown should fix that particular issue.

          Show
          jlowe Jason Lowe added a comment - Good catch, Daryn! I missed the memory-model races introduced by moving the stdout processing to a new thread. We could fix that with appropriate synchronization in the derived classes. However Shell is explicitly marked Public, so it's prudent to avoid disrupting that too much. It would be really nice to be able to kill the subprocess if the thread executing Shell is interrupted, but without moving the stdout processing to another thread I don't see a good way to do that. Reading from a stream doesn't seem to be interruptible in practice. If we can't make the Shell command reliably interruptible then the next best thing is to make sure we don't leave them lingering when this process exits so we can fix the problem described in YARN-5641 . Having a live Shell list that we can traverse on shutdown should fix that particular issue.
          Hide
          ebadger Eric Badger added a comment -

          Thanks, Jason Lowe, Daryn Sharp. I will work on a patch that implements a shutdown hook to destroy all of the created subprocesses.

          Show
          ebadger Eric Badger added a comment - Thanks, Jason Lowe , Daryn Sharp . I will work on a patch that implements a shutdown hook to destroy all of the created subprocesses.
          Hide
          ebadger Eric Badger added a comment -

          Attaching new patch that uses a shutdown hook to kill the child process that was spawned by shell. I tested this on a local cluster on the localizer use case from YARN-5641. I replaced the untar process with a sleep 1000000 process and confirmed that the sleep was killed immediately after the localizer. Before this patch, the localizer would shutdown without killing the sleep process.

          Jason Lowe, Daryn Sharp, please review

          Show
          ebadger Eric Badger added a comment - Attaching new patch that uses a shutdown hook to kill the child process that was spawned by shell. I tested this on a local cluster on the localizer use case from YARN-5641 . I replaced the untar process with a sleep 1000000 process and confirmed that the sleep was killed immediately after the localizer. Before this patch, the localizer would shutdown without killing the sleep process. Jason Lowe , Daryn Sharp , please review
          Hide
          hadoopqa Hadoop QA added a comment -
          -1 overall



          Vote Subsystem Runtime Comment
          0 reexec 0m 15s 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.
          +1 mvninstall 7m 10s trunk passed
          +1 compile 7m 26s trunk passed
          +1 checkstyle 0m 26s trunk passed
          +1 mvnsite 1m 7s trunk passed
          +1 mvneclipse 0m 14s trunk passed
          +1 findbugs 1m 32s trunk passed
          +1 javadoc 0m 45s trunk passed
          +1 mvninstall 0m 43s the patch passed
          +1 compile 8m 28s the patch passed
          +1 javac 8m 28s the patch passed
          -0 checkstyle 0m 24s hadoop-common-project/hadoop-common: The patch generated 1 new + 55 unchanged - 0 fixed = 56 total (was 55)
          +1 mvnsite 0m 58s the patch passed
          +1 mvneclipse 0m 13s the patch passed
          +1 whitespace 0m 0s The patch has no whitespace issues.
          +1 findbugs 1m 38s the patch passed
          +1 javadoc 0m 47s the patch passed
          -1 unit 7m 50s hadoop-common in the patch failed.
          +1 asflicense 0m 22s The patch does not generate ASF License warnings.
          41m 47s



          Reason Tests
          Failed junit tests hadoop.ha.TestZKFailoverController



          Subsystem Report/Notes
          Docker Image:yetus/hadoop:9560f25
          JIRA Issue HADOOP-13709
          JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12833453/HADOOP-13709.002.patch
          Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle
          uname Linux 7fb245007915 3.13.0-93-generic #140-Ubuntu SMP Mon Jul 18 21:21:05 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux
          Build tool maven
          Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh
          git revision trunk / 76cc84e
          Default Java 1.8.0_101
          findbugs v3.0.0
          checkstyle https://builds.apache.org/job/PreCommit-HADOOP-Build/10796/artifact/patchprocess/diff-checkstyle-hadoop-common-project_hadoop-common.txt
          unit https://builds.apache.org/job/PreCommit-HADOOP-Build/10796/artifact/patchprocess/patch-unit-hadoop-common-project_hadoop-common.txt
          Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/10796/testReport/
          modules C: hadoop-common-project/hadoop-common U: hadoop-common-project/hadoop-common
          Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/10796/console
          Powered by Apache Yetus 0.4.0-SNAPSHOT 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 15s 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. +1 mvninstall 7m 10s trunk passed +1 compile 7m 26s trunk passed +1 checkstyle 0m 26s trunk passed +1 mvnsite 1m 7s trunk passed +1 mvneclipse 0m 14s trunk passed +1 findbugs 1m 32s trunk passed +1 javadoc 0m 45s trunk passed +1 mvninstall 0m 43s the patch passed +1 compile 8m 28s the patch passed +1 javac 8m 28s the patch passed -0 checkstyle 0m 24s hadoop-common-project/hadoop-common: The patch generated 1 new + 55 unchanged - 0 fixed = 56 total (was 55) +1 mvnsite 0m 58s the patch passed +1 mvneclipse 0m 13s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. +1 findbugs 1m 38s the patch passed +1 javadoc 0m 47s the patch passed -1 unit 7m 50s hadoop-common in the patch failed. +1 asflicense 0m 22s The patch does not generate ASF License warnings. 41m 47s Reason Tests Failed junit tests hadoop.ha.TestZKFailoverController Subsystem Report/Notes Docker Image:yetus/hadoop:9560f25 JIRA Issue HADOOP-13709 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12833453/HADOOP-13709.002.patch Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle uname Linux 7fb245007915 3.13.0-93-generic #140-Ubuntu SMP Mon Jul 18 21:21:05 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh git revision trunk / 76cc84e Default Java 1.8.0_101 findbugs v3.0.0 checkstyle https://builds.apache.org/job/PreCommit-HADOOP-Build/10796/artifact/patchprocess/diff-checkstyle-hadoop-common-project_hadoop-common.txt unit https://builds.apache.org/job/PreCommit-HADOOP-Build/10796/artifact/patchprocess/patch-unit-hadoop-common-project_hadoop-common.txt Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/10796/testReport/ modules C: hadoop-common-project/hadoop-common U: hadoop-common-project/hadoop-common Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/10796/console Powered by Apache Yetus 0.4.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.
          Hide
          ebadger Eric Badger added a comment -

          The TestZKFailoverController failure is a transient. I tested locally and it didn't fail in 30 attempts.

          Show
          ebadger Eric Badger added a comment - The TestZKFailoverController failure is a transient. I tested locally and it didn't fail in 30 attempts.
          Hide
          daryn Daryn Sharp added a comment -

          This approach will cause a memory leak and eventually lead to an unrecoverable OOM. The shutdown thread instances and the Shell instances referenced by said threads will not be garbage collected until shutdown.

          A way to avoid this is Shell instances registering themselves in a static collection via the ctor. A single shutdown hook will destroy all the Shell instances in the collection. The remaining issue is removing completed Shell instances to avoid a similar leak. It's probably not safe to assume the Shell instance can be trusted to always remove itself so using a WeakHashMap is probably the safest approach.

          Show
          daryn Daryn Sharp added a comment - This approach will cause a memory leak and eventually lead to an unrecoverable OOM. The shutdown thread instances and the Shell instances referenced by said threads will not be garbage collected until shutdown. A way to avoid this is Shell instances registering themselves in a static collection via the ctor. A single shutdown hook will destroy all the Shell instances in the collection. The remaining issue is removing completed Shell instances to avoid a similar leak. It's probably not safe to assume the Shell instance can be trusted to always remove itself so using a WeakHashMap is probably the safest approach.
          Hide
          ebadger Eric Badger added a comment -

          Thanks for the review, Daryn Sharp! I addressed your comments and am attaching a patch with a single shutdown hook that iterates through a WeakHashMap to destroy all child processes. As before, I tested this out on the localizer using a local cluster with sleep in the place of untar. When the job was killed, the sleep process was also subsequently killed.

          Show
          ebadger Eric Badger added a comment - Thanks for the review, Daryn Sharp ! I addressed your comments and am attaching a patch with a single shutdown hook that iterates through a WeakHashMap to destroy all child processes. As before, I tested this out on the localizer using a local cluster with sleep in the place of untar . When the job was killed, the sleep process was also subsequently killed.
          Hide
          hadoopqa Hadoop QA added a comment -
          -1 overall



          Vote Subsystem Runtime Comment
          0 reexec 0m 14s 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.
          +1 mvninstall 6m 36s trunk passed
          +1 compile 6m 50s trunk passed
          +1 checkstyle 0m 24s trunk passed
          +1 mvnsite 0m 56s trunk passed
          +1 mvneclipse 0m 13s trunk passed
          +1 findbugs 1m 26s trunk passed
          +1 javadoc 0m 47s trunk passed
          +1 mvninstall 0m 44s the patch passed
          +1 compile 8m 7s the patch passed
          -1 javac 8m 7s root generated 1 new + 702 unchanged - 1 fixed = 703 total (was 703)
          -0 checkstyle 0m 26s hadoop-common-project/hadoop-common: The patch generated 2 new + 55 unchanged - 0 fixed = 57 total (was 55)
          +1 mvnsite 1m 0s the patch passed
          +1 mvneclipse 0m 13s the patch passed
          +1 whitespace 0m 0s The patch has no whitespace issues.
          +1 findbugs 1m 31s the patch passed
          +1 javadoc 0m 44s the patch passed
          +1 unit 8m 16s hadoop-common in the patch passed.
          +1 asflicense 0m 21s The patch does not generate ASF License warnings.
          40m 17s



          Subsystem Report/Notes
          Docker Image:yetus/hadoop:9560f25
          JIRA Issue HADOOP-13709
          JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12834693/HADOOP-13709.003.patch
          Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle
          uname Linux f97a654373a3 3.13.0-36-lowlatency #63-Ubuntu SMP PREEMPT Wed Sep 3 21:56:12 UTC 2014 x86_64 x86_64 x86_64 GNU/Linux
          Build tool maven
          Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh
          git revision trunk / f63cd78
          Default Java 1.8.0_101
          findbugs v3.0.0
          javac https://builds.apache.org/job/PreCommit-HADOOP-Build/10853/artifact/patchprocess/diff-compile-javac-root.txt
          checkstyle https://builds.apache.org/job/PreCommit-HADOOP-Build/10853/artifact/patchprocess/diff-checkstyle-hadoop-common-project_hadoop-common.txt
          Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/10853/testReport/
          modules C: hadoop-common-project/hadoop-common U: hadoop-common-project/hadoop-common
          Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/10853/console
          Powered by Apache Yetus 0.4.0-SNAPSHOT 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 14s 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. +1 mvninstall 6m 36s trunk passed +1 compile 6m 50s trunk passed +1 checkstyle 0m 24s trunk passed +1 mvnsite 0m 56s trunk passed +1 mvneclipse 0m 13s trunk passed +1 findbugs 1m 26s trunk passed +1 javadoc 0m 47s trunk passed +1 mvninstall 0m 44s the patch passed +1 compile 8m 7s the patch passed -1 javac 8m 7s root generated 1 new + 702 unchanged - 1 fixed = 703 total (was 703) -0 checkstyle 0m 26s hadoop-common-project/hadoop-common: The patch generated 2 new + 55 unchanged - 0 fixed = 57 total (was 55) +1 mvnsite 1m 0s the patch passed +1 mvneclipse 0m 13s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. +1 findbugs 1m 31s the patch passed +1 javadoc 0m 44s the patch passed +1 unit 8m 16s hadoop-common in the patch passed. +1 asflicense 0m 21s The patch does not generate ASF License warnings. 40m 17s Subsystem Report/Notes Docker Image:yetus/hadoop:9560f25 JIRA Issue HADOOP-13709 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12834693/HADOOP-13709.003.patch Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle uname Linux f97a654373a3 3.13.0-36-lowlatency #63-Ubuntu SMP PREEMPT Wed Sep 3 21:56:12 UTC 2014 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh git revision trunk / f63cd78 Default Java 1.8.0_101 findbugs v3.0.0 javac https://builds.apache.org/job/PreCommit-HADOOP-Build/10853/artifact/patchprocess/diff-compile-javac-root.txt checkstyle https://builds.apache.org/job/PreCommit-HADOOP-Build/10853/artifact/patchprocess/diff-checkstyle-hadoop-common-project_hadoop-common.txt Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/10853/testReport/ modules C: hadoop-common-project/hadoop-common U: hadoop-common-project/hadoop-common Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/10853/console Powered by Apache Yetus 0.4.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.
          Hide
          ebadger Eric Badger added a comment - - edited

          Taking Jason Lowe's advice and attaching a new patch that makes destroyChildProcesses() public. That way it can be called outside of the shutdown hook. This will be useful so that the localizer can kill its subprocesses and clean up after itself before the shutdown hook is called (this would be a follow-up change in YARN-5641).

          Show
          ebadger Eric Badger added a comment - - edited Taking Jason Lowe 's advice and attaching a new patch that makes destroyChildProcesses() public. That way it can be called outside of the shutdown hook. This will be useful so that the localizer can kill its subprocesses and clean up after itself before the shutdown hook is called (this would be a follow-up change in YARN-5641 ).
          Hide
          hadoopqa Hadoop QA added a comment -
          -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 appears to include 1 new or modified test files.
          +1 mvninstall 8m 8s trunk passed
          +1 compile 7m 23s trunk passed
          +1 checkstyle 0m 24s trunk passed
          +1 mvnsite 1m 1s trunk passed
          +1 mvneclipse 0m 12s trunk passed
          +1 findbugs 1m 26s trunk passed
          +1 javadoc 0m 43s trunk passed
          +1 mvninstall 0m 41s the patch passed
          +1 compile 7m 32s the patch passed
          -1 javac 7m 32s root generated 1 new + 700 unchanged - 1 fixed = 701 total (was 701)
          -0 checkstyle 0m 26s hadoop-common-project/hadoop-common: The patch generated 2 new + 55 unchanged - 0 fixed = 57 total (was 55)
          +1 mvnsite 1m 8s the patch passed
          +1 mvneclipse 0m 18s the patch passed
          +1 whitespace 0m 1s The patch has no whitespace issues.
          +1 findbugs 1m 45s the patch passed
          +1 javadoc 0m 45s the patch passed
          +1 unit 8m 58s hadoop-common in the patch passed.
          +1 asflicense 0m 26s The patch does not generate ASF License warnings.
          42m 55s



          Subsystem Report/Notes
          Docker Image:yetus/hadoop:9560f25
          JIRA Issue HADOOP-13709
          JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12835684/HADOOP-13709.004.patch
          Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle
          uname Linux a8aa9ea6906e 3.13.0-92-generic #139-Ubuntu SMP Tue Jun 28 20:42:26 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux
          Build tool maven
          Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh
          git revision trunk / 9e03ee5
          Default Java 1.8.0_101
          findbugs v3.0.0
          javac https://builds.apache.org/job/PreCommit-HADOOP-Build/10912/artifact/patchprocess/diff-compile-javac-root.txt
          checkstyle https://builds.apache.org/job/PreCommit-HADOOP-Build/10912/artifact/patchprocess/diff-checkstyle-hadoop-common-project_hadoop-common.txt
          Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/10912/testReport/
          modules C: hadoop-common-project/hadoop-common U: hadoop-common-project/hadoop-common
          Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/10912/console
          Powered by Apache Yetus 0.4.0-SNAPSHOT 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 16s 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. +1 mvninstall 8m 8s trunk passed +1 compile 7m 23s trunk passed +1 checkstyle 0m 24s trunk passed +1 mvnsite 1m 1s trunk passed +1 mvneclipse 0m 12s trunk passed +1 findbugs 1m 26s trunk passed +1 javadoc 0m 43s trunk passed +1 mvninstall 0m 41s the patch passed +1 compile 7m 32s the patch passed -1 javac 7m 32s root generated 1 new + 700 unchanged - 1 fixed = 701 total (was 701) -0 checkstyle 0m 26s hadoop-common-project/hadoop-common: The patch generated 2 new + 55 unchanged - 0 fixed = 57 total (was 55) +1 mvnsite 1m 8s the patch passed +1 mvneclipse 0m 18s the patch passed +1 whitespace 0m 1s The patch has no whitespace issues. +1 findbugs 1m 45s the patch passed +1 javadoc 0m 45s the patch passed +1 unit 8m 58s hadoop-common in the patch passed. +1 asflicense 0m 26s The patch does not generate ASF License warnings. 42m 55s Subsystem Report/Notes Docker Image:yetus/hadoop:9560f25 JIRA Issue HADOOP-13709 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12835684/HADOOP-13709.004.patch Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle uname Linux a8aa9ea6906e 3.13.0-92-generic #139-Ubuntu SMP Tue Jun 28 20:42:26 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh git revision trunk / 9e03ee5 Default Java 1.8.0_101 findbugs v3.0.0 javac https://builds.apache.org/job/PreCommit-HADOOP-Build/10912/artifact/patchprocess/diff-compile-javac-root.txt checkstyle https://builds.apache.org/job/PreCommit-HADOOP-Build/10912/artifact/patchprocess/diff-checkstyle-hadoop-common-project_hadoop-common.txt Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/10912/testReport/ modules C: hadoop-common-project/hadoop-common U: hadoop-common-project/hadoop-common Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/10912/console Powered by Apache Yetus 0.4.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.
          Hide
          ebadger Eric Badger added a comment -

          Jason Lowe, Andrew Wang, Daryn Sharp, do you mind reviewing the latest patch? Thanks

          Show
          ebadger Eric Badger added a comment - Jason Lowe , Andrew Wang , Daryn Sharp , do you mind reviewing the latest patch? Thanks
          Hide
          daryn Daryn Sharp added a comment -

          Please declare the map with types so you don't have to typecast the objects. That's probably the javac warning and style ding.

          Minor comments/suggestions. I'd call the method destroyAllProcesses. You could remove the explicit syncs when adding/removing if you use Collections.synchronizedMap.

          Show
          daryn Daryn Sharp added a comment - Please declare the map with types so you don't have to typecast the objects. That's probably the javac warning and style ding. Minor comments/suggestions. I'd call the method destroyAllProcesses . You could remove the explicit syncs when adding/removing if you use Collections.synchronizedMap .
          Hide
          ebadger Eric Badger added a comment -

          Thanks for the review, Daryn Sharp! Attaching new patch that uses a synchronizedMap and sets the type at allocation.

          Show
          ebadger Eric Badger added a comment - Thanks for the review, Daryn Sharp ! Attaching new patch that uses a synchronizedMap and sets the type at allocation.
          Hide
          hadoopqa Hadoop QA added a comment -
          -1 overall



          Vote Subsystem Runtime Comment
          0 reexec 0m 11s 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.
          +1 mvninstall 7m 46s trunk passed
          +1 compile 9m 35s trunk passed
          +1 checkstyle 0m 28s trunk passed
          +1 mvnsite 1m 1s trunk passed
          +1 mvneclipse 0m 17s trunk passed
          +1 findbugs 1m 31s trunk passed
          +1 javadoc 0m 51s trunk passed
          +1 mvninstall 0m 43s the patch passed
          +1 compile 10m 24s the patch passed
          +1 javac 10m 24s the patch passed
          -0 checkstyle 0m 30s hadoop-common-project/hadoop-common: The patch generated 2 new + 54 unchanged - 0 fixed = 56 total (was 54)
          +1 mvnsite 1m 1s the patch passed
          +1 mvneclipse 0m 18s the patch passed
          +1 whitespace 0m 0s The patch has no whitespace issues.
          +1 findbugs 1m 33s the patch passed
          +1 javadoc 0m 48s the patch passed
          -1 unit 11m 57s hadoop-common in the patch failed.
          +1 asflicense 1m 56s The patch does not generate ASF License warnings.
          52m 39s



          Reason Tests
          Failed junit tests hadoop.fs.TestDelegationTokenRenewer
            hadoop.fs.TestTrash
            hadoop.ha.TestZKFailoverController
            hadoop.net.TestClusterTopology



          Subsystem Report/Notes
          Docker Image:yetus/hadoop:a9ad5d6
          JIRA Issue HADOOP-13709
          JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12841089/HADOOP-13709.005.patch
          Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle
          uname Linux eef4591adf46 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 / 7c84871
          Default Java 1.8.0_111
          findbugs v3.0.0
          checkstyle https://builds.apache.org/job/PreCommit-HADOOP-Build/11167/artifact/patchprocess/diff-checkstyle-hadoop-common-project_hadoop-common.txt
          unit https://builds.apache.org/job/PreCommit-HADOOP-Build/11167/artifact/patchprocess/patch-unit-hadoop-common-project_hadoop-common.txt
          Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/11167/testReport/
          modules C: hadoop-common-project/hadoop-common U: hadoop-common-project/hadoop-common
          Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/11167/console
          Powered by Apache Yetus 0.4.0-SNAPSHOT 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 11s 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. +1 mvninstall 7m 46s trunk passed +1 compile 9m 35s trunk passed +1 checkstyle 0m 28s trunk passed +1 mvnsite 1m 1s trunk passed +1 mvneclipse 0m 17s trunk passed +1 findbugs 1m 31s trunk passed +1 javadoc 0m 51s trunk passed +1 mvninstall 0m 43s the patch passed +1 compile 10m 24s the patch passed +1 javac 10m 24s the patch passed -0 checkstyle 0m 30s hadoop-common-project/hadoop-common: The patch generated 2 new + 54 unchanged - 0 fixed = 56 total (was 54) +1 mvnsite 1m 1s the patch passed +1 mvneclipse 0m 18s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. +1 findbugs 1m 33s the patch passed +1 javadoc 0m 48s the patch passed -1 unit 11m 57s hadoop-common in the patch failed. +1 asflicense 1m 56s The patch does not generate ASF License warnings. 52m 39s Reason Tests Failed junit tests hadoop.fs.TestDelegationTokenRenewer   hadoop.fs.TestTrash   hadoop.ha.TestZKFailoverController   hadoop.net.TestClusterTopology Subsystem Report/Notes Docker Image:yetus/hadoop:a9ad5d6 JIRA Issue HADOOP-13709 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12841089/HADOOP-13709.005.patch Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle uname Linux eef4591adf46 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 / 7c84871 Default Java 1.8.0_111 findbugs v3.0.0 checkstyle https://builds.apache.org/job/PreCommit-HADOOP-Build/11167/artifact/patchprocess/diff-checkstyle-hadoop-common-project_hadoop-common.txt unit https://builds.apache.org/job/PreCommit-HADOOP-Build/11167/artifact/patchprocess/patch-unit-hadoop-common-project_hadoop-common.txt Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/11167/testReport/ modules C: hadoop-common-project/hadoop-common U: hadoop-common-project/hadoop-common Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/11167/console Powered by Apache Yetus 0.4.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.
          Hide
          ebadger Eric Badger added a comment -

          Fixing minor checkstyle issues

          Show
          ebadger Eric Badger added a comment - Fixing minor checkstyle issues
          Hide
          hadoopqa Hadoop QA added a comment -
          +1 overall



          Vote Subsystem Runtime Comment
          0 reexec 0m 15s 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.
          +1 mvninstall 7m 50s trunk passed
          +1 compile 9m 54s trunk passed
          +1 checkstyle 0m 30s trunk passed
          +1 mvnsite 1m 4s trunk passed
          +1 mvneclipse 0m 19s trunk passed
          +1 findbugs 1m 27s trunk passed
          +1 javadoc 0m 46s trunk passed
          +1 mvninstall 0m 38s the patch passed
          +1 compile 9m 40s the patch passed
          +1 javac 9m 40s the patch passed
          +1 checkstyle 0m 28s the patch passed
          +1 mvnsite 1m 0s the patch passed
          +1 mvneclipse 0m 18s the patch passed
          +1 whitespace 0m 0s The patch has no whitespace issues.
          +1 findbugs 1m 38s the patch passed
          +1 javadoc 0m 49s the patch passed
          +1 unit 8m 23s hadoop-common in the patch passed.
          +1 asflicense 0m 35s The patch does not generate ASF License warnings.
          47m 24s



          Subsystem Report/Notes
          Docker Image:yetus/hadoop:a9ad5d6
          JIRA Issue HADOOP-13709
          JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12841127/HADOOP-13709.006.patch
          Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle
          uname Linux a83da421c5e2 3.13.0-93-generic #140-Ubuntu SMP Mon Jul 18 21:21:05 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux
          Build tool maven
          Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh
          git revision trunk / 4fca94f
          Default Java 1.8.0_111
          findbugs v3.0.0
          Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/11168/testReport/
          modules C: hadoop-common-project/hadoop-common U: hadoop-common-project/hadoop-common
          Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/11168/console
          Powered by Apache Yetus 0.4.0-SNAPSHOT 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 15s 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. +1 mvninstall 7m 50s trunk passed +1 compile 9m 54s trunk passed +1 checkstyle 0m 30s trunk passed +1 mvnsite 1m 4s trunk passed +1 mvneclipse 0m 19s trunk passed +1 findbugs 1m 27s trunk passed +1 javadoc 0m 46s trunk passed +1 mvninstall 0m 38s the patch passed +1 compile 9m 40s the patch passed +1 javac 9m 40s the patch passed +1 checkstyle 0m 28s the patch passed +1 mvnsite 1m 0s the patch passed +1 mvneclipse 0m 18s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. +1 findbugs 1m 38s the patch passed +1 javadoc 0m 49s the patch passed +1 unit 8m 23s hadoop-common in the patch passed. +1 asflicense 0m 35s The patch does not generate ASF License warnings. 47m 24s Subsystem Report/Notes Docker Image:yetus/hadoop:a9ad5d6 JIRA Issue HADOOP-13709 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12841127/HADOOP-13709.006.patch Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle uname Linux a83da421c5e2 3.13.0-93-generic #140-Ubuntu SMP Mon Jul 18 21:21:05 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh git revision trunk / 4fca94f Default Java 1.8.0_111 findbugs v3.0.0 Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/11168/testReport/ modules C: hadoop-common-project/hadoop-common U: hadoop-common-project/hadoop-common Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/11168/console Powered by Apache Yetus 0.4.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.
          Hide
          daryn Daryn Sharp added a comment -

          +1 assuming no objections from Jason Lowe.

          Show
          daryn Daryn Sharp added a comment - +1 assuming no objections from Jason Lowe .
          Hide
          jlowe Jason Lowe added a comment -

          The synchronized map needs to be locked explicitly when iterated otherwise we have concurrency issues if some other thread tries to update this map while we're walking it during the shutdown hook processing.

          The unit test is racy because it assumes a 250ms sleep is enough to get the sleep processes started. It would be better to poll for getProcess() being non-null for the two executors. GenericTestUtils.waitFor would be useful here and can also replace the manually-coded poll loop.

          Show
          jlowe Jason Lowe added a comment - The synchronized map needs to be locked explicitly when iterated otherwise we have concurrency issues if some other thread tries to update this map while we're walking it during the shutdown hook processing. The unit test is racy because it assumes a 250ms sleep is enough to get the sleep processes started. It would be better to poll for getProcess() being non-null for the two executors. GenericTestUtils.waitFor would be useful here and can also replace the manually-coded poll loop.
          Hide
          ebadger Eric Badger added a comment -

          Thanks for the comments, Jason Lowe!

          The synchronized map needs to be locked explicitly when iterated otherwise we have concurrency issues if some other thread tries to update this map while we're walking it during the shutdown hook processing.

          Good catch, I put this back in in the latest patch.

          The unit test is racy because it assumes a 250ms sleep is enough to get the sleep processes started. It would be better to poll for getProcess() being non-null for the two executors. GenericTestUtils.waitFor would be useful here and can also replace the manually-coded poll loop.

          Changed all waits to use GenericTestUtils.waitFor in the latest patch.

          Show
          ebadger Eric Badger added a comment - Thanks for the comments, Jason Lowe ! The synchronized map needs to be locked explicitly when iterated otherwise we have concurrency issues if some other thread tries to update this map while we're walking it during the shutdown hook processing. Good catch, I put this back in in the latest patch. The unit test is racy because it assumes a 250ms sleep is enough to get the sleep processes started. It would be better to poll for getProcess() being non-null for the two executors. GenericTestUtils.waitFor would be useful here and can also replace the manually-coded poll loop. Changed all waits to use GenericTestUtils.waitFor in the latest patch.
          Hide
          hadoopqa Hadoop QA added a comment -
          +1 overall



          Vote Subsystem Runtime Comment
          0 reexec 0m 14s 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.
          +1 mvninstall 6m 53s trunk passed
          +1 compile 9m 29s trunk passed
          +1 checkstyle 0m 28s trunk passed
          +1 mvnsite 1m 1s trunk passed
          +1 mvneclipse 0m 19s trunk passed
          +1 findbugs 1m 24s trunk passed
          +1 javadoc 0m 46s trunk passed
          +1 mvninstall 0m 36s the patch passed
          +1 compile 9m 7s the patch passed
          +1 javac 9m 7s the patch passed
          +1 checkstyle 0m 29s the patch passed
          +1 mvnsite 0m 59s the patch passed
          +1 mvneclipse 0m 18s the patch passed
          +1 whitespace 0m 0s The patch has no whitespace issues.
          +1 findbugs 1m 38s the patch passed
          +1 javadoc 0m 46s the patch passed
          +1 unit 7m 40s hadoop-common in the patch passed.
          +1 asflicense 0m 31s The patch does not generate ASF License warnings.
          44m 25s



          Subsystem Report/Notes
          Docker Image:yetus/hadoop:a9ad5d6
          JIRA Issue HADOOP-13709
          JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12841368/HADOOP-13709.007.patch
          Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle
          uname Linux c88eb8f039db 3.13.0-93-generic #140-Ubuntu SMP Mon Jul 18 21:21:05 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux
          Build tool maven
          Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh
          git revision trunk / 19f373a
          Default Java 1.8.0_111
          findbugs v3.0.0
          Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/11178/testReport/
          modules C: hadoop-common-project/hadoop-common U: hadoop-common-project/hadoop-common
          Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/11178/console
          Powered by Apache Yetus 0.4.0-SNAPSHOT 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 14s 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. +1 mvninstall 6m 53s trunk passed +1 compile 9m 29s trunk passed +1 checkstyle 0m 28s trunk passed +1 mvnsite 1m 1s trunk passed +1 mvneclipse 0m 19s trunk passed +1 findbugs 1m 24s trunk passed +1 javadoc 0m 46s trunk passed +1 mvninstall 0m 36s the patch passed +1 compile 9m 7s the patch passed +1 javac 9m 7s the patch passed +1 checkstyle 0m 29s the patch passed +1 mvnsite 0m 59s the patch passed +1 mvneclipse 0m 18s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. +1 findbugs 1m 38s the patch passed +1 javadoc 0m 46s the patch passed +1 unit 7m 40s hadoop-common in the patch passed. +1 asflicense 0m 31s The patch does not generate ASF License warnings. 44m 25s Subsystem Report/Notes Docker Image:yetus/hadoop:a9ad5d6 JIRA Issue HADOOP-13709 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12841368/HADOOP-13709.007.patch Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle uname Linux c88eb8f039db 3.13.0-93-generic #140-Ubuntu SMP Mon Jul 18 21:21:05 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh git revision trunk / 19f373a Default Java 1.8.0_111 findbugs v3.0.0 Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/11178/testReport/ modules C: hadoop-common-project/hadoop-common U: hadoop-common-project/hadoop-common Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/11178/console Powered by Apache Yetus 0.4.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.
          Hide
          jlowe Jason Lowe added a comment -

          The synchronized blocks are unnecessary on the get and put methods for synchronized map. That's what a synchronized map brings to the table over a normal map – it adds a lock to those methods. However a synchronized map cannot auto-synchronize iteration which is why we need to explicitly lock it when walking it.

          It'd also be nice t mark process1 and process2 as final in the unit test since otherwise the patch will only compile on JDK8 or later (i.e.: only trunk at this point).

          Show
          jlowe Jason Lowe added a comment - The synchronized blocks are unnecessary on the get and put methods for synchronized map. That's what a synchronized map brings to the table over a normal map – it adds a lock to those methods. However a synchronized map cannot auto-synchronize iteration which is why we need to explicitly lock it when walking it. It'd also be nice t mark process1 and process2 as final in the unit test since otherwise the patch will only compile on JDK8 or later (i.e.: only trunk at this point).
          Hide
          ebadger Eric Badger added a comment -

          That all makes sense, Jason Lowe. I believe that I've fixed those all now. I left the synchronized map in and just got rid of the redundant synchronized blocks. Let me know if you'd rather use a regular map along with the synchronized blocks.

          Show
          ebadger Eric Badger added a comment - That all makes sense, Jason Lowe . I believe that I've fixed those all now. I left the synchronized map in and just got rid of the redundant synchronized blocks. Let me know if you'd rather use a regular map along with the synchronized blocks.
          Hide
          hadoopqa Hadoop QA added a comment -
          +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 appears to include 1 new or modified test files.
          +1 mvninstall 6m 44s trunk passed
          +1 compile 9m 29s trunk passed
          +1 checkstyle 0m 29s trunk passed
          +1 mvnsite 1m 1s trunk passed
          +1 mvneclipse 0m 17s trunk passed
          +1 findbugs 1m 23s trunk passed
          +1 javadoc 0m 48s trunk passed
          +1 mvninstall 0m 37s the patch passed
          +1 compile 9m 8s the patch passed
          +1 javac 9m 8s the patch passed
          +1 checkstyle 0m 28s the patch passed
          +1 mvnsite 0m 58s the patch passed
          +1 mvneclipse 0m 18s the patch passed
          +1 whitespace 0m 0s The patch has no whitespace issues.
          +1 findbugs 1m 31s the patch passed
          +1 javadoc 0m 47s the patch passed
          +1 unit 7m 36s hadoop-common in the patch passed.
          +1 asflicense 0m 31s The patch does not generate ASF License warnings.
          44m 2s



          Subsystem Report/Notes
          Docker Image:yetus/hadoop:a9ad5d6
          JIRA Issue HADOOP-13709
          JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12841534/HADOOP-13709.008.patch
          Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle
          uname Linux a42c27d58596 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 / 0cfd7ad
          Default Java 1.8.0_111
          findbugs v3.0.0
          Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/11185/testReport/
          modules C: hadoop-common-project/hadoop-common U: hadoop-common-project/hadoop-common
          Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/11185/console
          Powered by Apache Yetus 0.4.0-SNAPSHOT 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 12s 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. +1 mvninstall 6m 44s trunk passed +1 compile 9m 29s trunk passed +1 checkstyle 0m 29s trunk passed +1 mvnsite 1m 1s trunk passed +1 mvneclipse 0m 17s trunk passed +1 findbugs 1m 23s trunk passed +1 javadoc 0m 48s trunk passed +1 mvninstall 0m 37s the patch passed +1 compile 9m 8s the patch passed +1 javac 9m 8s the patch passed +1 checkstyle 0m 28s the patch passed +1 mvnsite 0m 58s the patch passed +1 mvneclipse 0m 18s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. +1 findbugs 1m 31s the patch passed +1 javadoc 0m 47s the patch passed +1 unit 7m 36s hadoop-common in the patch passed. +1 asflicense 0m 31s The patch does not generate ASF License warnings. 44m 2s Subsystem Report/Notes Docker Image:yetus/hadoop:a9ad5d6 JIRA Issue HADOOP-13709 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12841534/HADOOP-13709.008.patch Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle uname Linux a42c27d58596 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 / 0cfd7ad Default Java 1.8.0_111 findbugs v3.0.0 Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/11185/testReport/ modules C: hadoop-common-project/hadoop-common U: hadoop-common-project/hadoop-common Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/11185/console Powered by Apache Yetus 0.4.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.
          Hide
          jlowe Jason Lowe added a comment -

          Thanks for updating the patch! Synchronization changes look good.

          Thinking about the patch further, I believe this will break YARN nodemanager work-preserving restart. Currently the nodemanager does not kill the subprocesses when work-preserving restart is enabled, but this kill-all-on-shutdown feature will do it anyway. Therefore minimally I think we need to change it so the shell is capable of tracking shell processes but doesn't always kill them on shutdown. Anything that needs to kill things on shutdown (i.e.: the YARN localizer problematic case that caused this to be filed) can register their own shutdown hook to call Shell.destroyAllProcesses. Since this interface will be public, it would be good to provide some javadoc for it.

          Show
          jlowe Jason Lowe added a comment - Thanks for updating the patch! Synchronization changes look good. Thinking about the patch further, I believe this will break YARN nodemanager work-preserving restart. Currently the nodemanager does not kill the subprocesses when work-preserving restart is enabled, but this kill-all-on-shutdown feature will do it anyway. Therefore minimally I think we need to change it so the shell is capable of tracking shell processes but doesn't always kill them on shutdown. Anything that needs to kill things on shutdown (i.e.: the YARN localizer problematic case that caused this to be filed) can register their own shutdown hook to call Shell.destroyAllProcesses. Since this interface will be public, it would be good to provide some javadoc for it.
          Hide
          ebadger Eric Badger added a comment - - edited

          Uploading a new patch.
          Jason Lowe, I took out the static block that registers the shutdown hook in Shell. We can add this shutdown hook for the localizer via YARN-5641. Also added a javadoc for destroyAllProcesses.

          Show
          ebadger Eric Badger added a comment - - edited Uploading a new patch. Jason Lowe , I took out the static block that registers the shutdown hook in Shell . We can add this shutdown hook for the localizer via YARN-5641 . Also added a javadoc for destroyAllProcesses .
          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.
          +1 mvninstall 6m 56s trunk passed
          +1 compile 10m 43s trunk passed
          +1 checkstyle 0m 30s trunk passed
          +1 mvnsite 1m 7s trunk passed
          +1 mvneclipse 0m 16s trunk passed
          +1 findbugs 1m 26s trunk passed
          +1 javadoc 0m 50s trunk passed
          +1 mvninstall 0m 39s the patch passed
          +1 compile 10m 30s the patch passed
          +1 javac 10m 30s the patch passed
          -0 checkstyle 0m 29s hadoop-common-project/hadoop-common: The patch generated 2 new + 54 unchanged - 0 fixed = 56 total (was 54)
          +1 mvnsite 1m 1s the patch passed
          +1 mvneclipse 0m 17s the patch passed
          +1 whitespace 0m 0s The patch has no whitespace issues.
          +1 findbugs 1m 49s the patch passed
          +1 javadoc 0m 49s the patch passed
          -1 unit 9m 14s hadoop-common in the patch failed.
          +1 asflicense 0m 32s The patch does not generate ASF License warnings.
          49m 10s



          Reason Tests
          Failed junit tests hadoop.ipc.TestIPC



          Subsystem Report/Notes
          Docker Image:yetus/hadoop:a9ad5d6
          JIRA Issue HADOOP-13709
          JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12841553/HADOOP-13709.009.patch
          Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle
          uname Linux cf19f8246344 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 / c7ff34f
          Default Java 1.8.0_111
          findbugs v3.0.0
          checkstyle https://builds.apache.org/job/PreCommit-HADOOP-Build/11187/artifact/patchprocess/diff-checkstyle-hadoop-common-project_hadoop-common.txt
          unit https://builds.apache.org/job/PreCommit-HADOOP-Build/11187/artifact/patchprocess/patch-unit-hadoop-common-project_hadoop-common.txt
          Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/11187/testReport/
          modules C: hadoop-common-project/hadoop-common U: hadoop-common-project/hadoop-common
          Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/11187/console
          Powered by Apache Yetus 0.4.0-SNAPSHOT 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. +1 mvninstall 6m 56s trunk passed +1 compile 10m 43s trunk passed +1 checkstyle 0m 30s trunk passed +1 mvnsite 1m 7s trunk passed +1 mvneclipse 0m 16s trunk passed +1 findbugs 1m 26s trunk passed +1 javadoc 0m 50s trunk passed +1 mvninstall 0m 39s the patch passed +1 compile 10m 30s the patch passed +1 javac 10m 30s the patch passed -0 checkstyle 0m 29s hadoop-common-project/hadoop-common: The patch generated 2 new + 54 unchanged - 0 fixed = 56 total (was 54) +1 mvnsite 1m 1s the patch passed +1 mvneclipse 0m 17s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. +1 findbugs 1m 49s the patch passed +1 javadoc 0m 49s the patch passed -1 unit 9m 14s hadoop-common in the patch failed. +1 asflicense 0m 32s The patch does not generate ASF License warnings. 49m 10s Reason Tests Failed junit tests hadoop.ipc.TestIPC Subsystem Report/Notes Docker Image:yetus/hadoop:a9ad5d6 JIRA Issue HADOOP-13709 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12841553/HADOOP-13709.009.patch Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle uname Linux cf19f8246344 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 / c7ff34f Default Java 1.8.0_111 findbugs v3.0.0 checkstyle https://builds.apache.org/job/PreCommit-HADOOP-Build/11187/artifact/patchprocess/diff-checkstyle-hadoop-common-project_hadoop-common.txt unit https://builds.apache.org/job/PreCommit-HADOOP-Build/11187/artifact/patchprocess/patch-unit-hadoop-common-project_hadoop-common.txt Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/11187/testReport/ modules C: hadoop-common-project/hadoop-common U: hadoop-common-project/hadoop-common Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/11187/console Powered by Apache Yetus 0.4.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.
          Hide
          jlowe Jason Lowe added a comment -

          Thanks for updating the patch! The unit test failure appears to be unrelated. It would be good to cleanup the checkstyle line length nits. Also there's a missing space for "<code>Shell</code>processes".

          Show
          jlowe Jason Lowe added a comment - Thanks for updating the patch! The unit test failure appears to be unrelated. It would be good to cleanup the checkstyle line length nits. Also there's a missing space for "<code>Shell</code>processes".
          Hide
          ebadger Eric Badger added a comment -

          Fixed checkstyle issues

          Show
          ebadger Eric Badger added a comment - Fixed checkstyle issues
          Hide
          hadoopqa Hadoop QA added a comment -
          -1 overall



          Vote Subsystem Runtime Comment
          0 reexec 0m 15s 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.
          +1 mvninstall 7m 31s trunk passed
          +1 compile 10m 35s trunk passed
          +1 checkstyle 0m 29s trunk passed
          +1 mvnsite 1m 1s trunk passed
          +1 mvneclipse 0m 18s trunk passed
          +1 findbugs 1m 24s trunk passed
          +1 javadoc 0m 47s trunk passed
          +1 mvninstall 0m 36s the patch passed
          +1 compile 9m 6s the patch passed
          +1 javac 9m 6s the patch passed
          +1 checkstyle 0m 28s the patch passed
          +1 mvnsite 1m 1s the patch passed
          +1 mvneclipse 0m 18s the patch passed
          +1 whitespace 0m 0s The patch has no whitespace issues.
          +1 findbugs 1m 33s the patch passed
          +1 javadoc 0m 47s the patch passed
          -1 unit 7m 54s hadoop-common in the patch failed.
          +1 asflicense 0m 31s The patch does not generate ASF License warnings.
          46m 19s



          Reason Tests
          Failed junit tests hadoop.security.token.delegation.web.TestWebDelegationToken



          Subsystem Report/Notes
          Docker Image:yetus/hadoop:a9ad5d6
          JIRA Issue HADOOP-13709
          JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12842874/HADOOP-13709.009.patch
          Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle
          uname Linux 817fea1be363 3.13.0-103-generic #150-Ubuntu SMP Thu Nov 24 10:34:17 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux
          Build tool maven
          Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh
          git revision trunk / f66f618
          Default Java 1.8.0_111
          findbugs v3.0.0
          unit https://builds.apache.org/job/PreCommit-HADOOP-Build/11253/artifact/patchprocess/patch-unit-hadoop-common-project_hadoop-common.txt
          Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/11253/testReport/
          modules C: hadoop-common-project/hadoop-common U: hadoop-common-project/hadoop-common
          Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/11253/console
          Powered by Apache Yetus 0.5.0-SNAPSHOT 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 15s 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. +1 mvninstall 7m 31s trunk passed +1 compile 10m 35s trunk passed +1 checkstyle 0m 29s trunk passed +1 mvnsite 1m 1s trunk passed +1 mvneclipse 0m 18s trunk passed +1 findbugs 1m 24s trunk passed +1 javadoc 0m 47s trunk passed +1 mvninstall 0m 36s the patch passed +1 compile 9m 6s the patch passed +1 javac 9m 6s the patch passed +1 checkstyle 0m 28s the patch passed +1 mvnsite 1m 1s the patch passed +1 mvneclipse 0m 18s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. +1 findbugs 1m 33s the patch passed +1 javadoc 0m 47s the patch passed -1 unit 7m 54s hadoop-common in the patch failed. +1 asflicense 0m 31s The patch does not generate ASF License warnings. 46m 19s Reason Tests Failed junit tests hadoop.security.token.delegation.web.TestWebDelegationToken Subsystem Report/Notes Docker Image:yetus/hadoop:a9ad5d6 JIRA Issue HADOOP-13709 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12842874/HADOOP-13709.009.patch Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle uname Linux 817fea1be363 3.13.0-103-generic #150-Ubuntu SMP Thu Nov 24 10:34:17 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh git revision trunk / f66f618 Default Java 1.8.0_111 findbugs v3.0.0 unit https://builds.apache.org/job/PreCommit-HADOOP-Build/11253/artifact/patchprocess/patch-unit-hadoop-common-project_hadoop-common.txt Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/11253/testReport/ modules C: hadoop-common-project/hadoop-common U: hadoop-common-project/hadoop-common Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/11253/console Powered by Apache Yetus 0.5.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.
          Hide
          ebadger Eric Badger added a comment -

          The test failures are related to HADOOP-13890/HADOOP-13565 and are unrelated to the patch

          Show
          ebadger Eric Badger added a comment - The test failures are related to HADOOP-13890 / HADOOP-13565 and are unrelated to the patch
          Hide
          jlowe Jason Lowe added a comment -

          +1 lgtm. Committing this.

          Show
          jlowe Jason Lowe added a comment - +1 lgtm. Committing this.
          Hide
          jlowe Jason Lowe added a comment -

          Thanks to Eric Badger for the contribution and to Andrew Wang and Daryn Sharp for additional review! I committed this to trunk and branch-2.

          Show
          jlowe Jason Lowe added a comment - Thanks to Eric Badger for the contribution and to Andrew Wang and Daryn Sharp for additional review! I committed this to trunk and branch-2.
          Hide
          ebadger Eric Badger added a comment -

          Fantastic! Thanks, Jason Lowe, Andrew Wang, Daryn Sharp! I'll put up an associated patch on YARN-5641 to add it to the localizer.

          Show
          ebadger Eric Badger added a comment - Fantastic! Thanks, Jason Lowe , Andrew Wang , Daryn Sharp ! I'll put up an associated patch on YARN-5641 to add it to the localizer.
          Hide
          hudson Hudson added a comment -

          SUCCESS: Integrated in Jenkins build Hadoop-trunk-Commit #10991 (See https://builds.apache.org/job/Hadoop-trunk-Commit/10991/)
          HADOOP-13709. Ability to clean up subprocesses spawned by Shell when the (jlowe: rev 9947aeb60c3dd075544866fd6e4dab0ad8b4afa2)

          • (edit) hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/util/TestShell.java
          • (edit) hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/util/Shell.java
          Show
          hudson Hudson added a comment - SUCCESS: Integrated in Jenkins build Hadoop-trunk-Commit #10991 (See https://builds.apache.org/job/Hadoop-trunk-Commit/10991/ ) HADOOP-13709 . Ability to clean up subprocesses spawned by Shell when the (jlowe: rev 9947aeb60c3dd075544866fd6e4dab0ad8b4afa2) (edit) hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/util/TestShell.java (edit) hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/util/Shell.java
          Hide
          jojochuang Wei-Chiu Chuang added a comment -

          The branch-2 patch breaks with Java 7: the Process.isAlive() is a Java 8 API.

          [ERROR] Failed to execute goal org.apache.maven.plugins:maven-compiler-plugin:3.1:testCompile (default-testCompile) on project hadoop-common: Compilation failure: Compilation failure:
          [ERROR] /Users/weichiu/sandbox/hadoop/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/util/TestShell.java:[533,25] cannot find symbol
          [ERROR] symbol: method isAlive()
          [ERROR] location: variable process1 of type java.lang.Process
          [ERROR] /Users/weichiu/sandbox/hadoop/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/util/TestShell.java:[540,25] cannot find symbol
          [ERROR] symbol: method isAlive()
          [ERROR] location: variable process2 of type java.lang.Process
          [ERROR] /Users/weichiu/sandbox/hadoop/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/util/TestShell.java:[544,68] cannot find symbol
          [ERROR] symbol: method isAlive()
          [ERROR] location: variable process1 of type java.lang.Process
          [ERROR] /Users/weichiu/sandbox/hadoop/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/util/TestShell.java:[545,68] cannot find symbol
          [ERROR] symbol: method isAlive()
          [ERROR] location: variable process2 of type java.lang.Process

          Show
          jojochuang Wei-Chiu Chuang added a comment - The branch-2 patch breaks with Java 7: the Process.isAlive() is a Java 8 API. [ERROR] Failed to execute goal org.apache.maven.plugins:maven-compiler-plugin:3.1:testCompile (default-testCompile) on project hadoop-common: Compilation failure: Compilation failure: [ERROR] /Users/weichiu/sandbox/hadoop/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/util/TestShell.java: [533,25] cannot find symbol [ERROR] symbol: method isAlive() [ERROR] location: variable process1 of type java.lang.Process [ERROR] /Users/weichiu/sandbox/hadoop/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/util/TestShell.java: [540,25] cannot find symbol [ERROR] symbol: method isAlive() [ERROR] location: variable process2 of type java.lang.Process [ERROR] /Users/weichiu/sandbox/hadoop/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/util/TestShell.java: [544,68] cannot find symbol [ERROR] symbol: method isAlive() [ERROR] location: variable process1 of type java.lang.Process [ERROR] /Users/weichiu/sandbox/hadoop/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/util/TestShell.java: [545,68] cannot find symbol [ERROR] symbol: method isAlive() [ERROR] location: variable process2 of type java.lang.Process
          Hide
          jojochuang Wei-Chiu Chuang added a comment -

          Eric Badger I am sorry to reopen this issue. Would you please help fix the compilation issue with Java 7? Or should we consider revert this commit?

          Show
          jojochuang Wei-Chiu Chuang added a comment - Eric Badger I am sorry to reopen this issue. Would you please help fix the compilation issue with Java 7? Or should we consider revert this commit?
          Hide
          jlowe Jason Lowe added a comment -

          Apologies for missing the JDK7 issue on branch-2. Reverting the branch-2 version for now.

          Show
          jlowe Jason Lowe added a comment - Apologies for missing the JDK7 issue on branch-2. Reverting the branch-2 version for now.
          Hide
          ebadger Eric Badger added a comment -

          Sorry about breaking branch-2, Wei-Chiu Chuang! Putting up a new patch that should be compatible with JDK7.

          Show
          ebadger Eric Badger added a comment - Sorry about breaking branch-2, Wei-Chiu Chuang ! Putting up a new patch that should be compatible with JDK7.
          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.
          +1 mvninstall 6m 38s branch-2 passed
          +1 compile 5m 33s branch-2 passed with JDK v1.8.0_111
          +1 compile 6m 26s branch-2 passed with JDK v1.7.0_121
          +1 checkstyle 0m 26s branch-2 passed
          +1 mvnsite 0m 56s branch-2 passed
          +1 mvneclipse 0m 15s branch-2 passed
          +1 findbugs 1m 39s branch-2 passed
          +1 javadoc 0m 47s branch-2 passed with JDK v1.8.0_111
          +1 javadoc 0m 59s branch-2 passed with JDK v1.7.0_121
          +1 mvninstall 0m 39s the patch passed
          +1 compile 5m 45s the patch passed with JDK v1.8.0_111
          +1 javac 5m 45s the patch passed
          +1 compile 6m 55s the patch passed with JDK v1.7.0_121
          +1 javac 6m 55s the patch passed
          +1 checkstyle 0m 25s the patch passed
          +1 mvnsite 0m 58s the patch passed
          +1 mvneclipse 0m 14s the patch passed
          +1 whitespace 0m 0s The patch has no whitespace issues.
          +1 findbugs 1m 54s the patch passed
          +1 javadoc 0m 49s the patch passed with JDK v1.8.0_111
          +1 javadoc 0m 59s the patch passed with JDK v1.7.0_121
          +1 unit 10m 59s hadoop-common in the patch passed with JDK v1.7.0_121.
          +1 asflicense 0m 21s The patch does not generate ASF License warnings.
          64m 52s



          Reason Tests
          JDK v1.8.0_111 Failed junit tests hadoop.net.TestDNS



          Subsystem Report/Notes
          Docker Image:yetus/hadoop:b59b8b7
          JIRA Issue HADOOP-13709
          JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12843426/HADOOP-13709-branch-2.010.patch
          Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle
          uname Linux 0f6d39935171 3.13.0-103-generic #150-Ubuntu SMP Thu Nov 24 10:34:17 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux
          Build tool maven
          Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh
          git revision branch-2 / eaa50a1
          Default Java 1.7.0_121
          Multi-JDK versions /usr/lib/jvm/java-8-oracle:1.8.0_111 /usr/lib/jvm/java-7-openjdk-amd64:1.7.0_121
          findbugs v3.0.0
          JDK v1.7.0_121 Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/11285/testReport/
          modules C: hadoop-common-project/hadoop-common U: hadoop-common-project/hadoop-common
          Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/11285/console
          Powered by Apache Yetus 0.5.0-SNAPSHOT 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. +1 mvninstall 6m 38s branch-2 passed +1 compile 5m 33s branch-2 passed with JDK v1.8.0_111 +1 compile 6m 26s branch-2 passed with JDK v1.7.0_121 +1 checkstyle 0m 26s branch-2 passed +1 mvnsite 0m 56s branch-2 passed +1 mvneclipse 0m 15s branch-2 passed +1 findbugs 1m 39s branch-2 passed +1 javadoc 0m 47s branch-2 passed with JDK v1.8.0_111 +1 javadoc 0m 59s branch-2 passed with JDK v1.7.0_121 +1 mvninstall 0m 39s the patch passed +1 compile 5m 45s the patch passed with JDK v1.8.0_111 +1 javac 5m 45s the patch passed +1 compile 6m 55s the patch passed with JDK v1.7.0_121 +1 javac 6m 55s the patch passed +1 checkstyle 0m 25s the patch passed +1 mvnsite 0m 58s the patch passed +1 mvneclipse 0m 14s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. +1 findbugs 1m 54s the patch passed +1 javadoc 0m 49s the patch passed with JDK v1.8.0_111 +1 javadoc 0m 59s the patch passed with JDK v1.7.0_121 +1 unit 10m 59s hadoop-common in the patch passed with JDK v1.7.0_121. +1 asflicense 0m 21s The patch does not generate ASF License warnings. 64m 52s Reason Tests JDK v1.8.0_111 Failed junit tests hadoop.net.TestDNS Subsystem Report/Notes Docker Image:yetus/hadoop:b59b8b7 JIRA Issue HADOOP-13709 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12843426/HADOOP-13709-branch-2.010.patch Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle uname Linux 0f6d39935171 3.13.0-103-generic #150-Ubuntu SMP Thu Nov 24 10:34:17 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh git revision branch-2 / eaa50a1 Default Java 1.7.0_121 Multi-JDK versions /usr/lib/jvm/java-8-oracle:1.8.0_111 /usr/lib/jvm/java-7-openjdk-amd64:1.7.0_121 findbugs v3.0.0 JDK v1.7.0_121 Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/11285/testReport/ modules C: hadoop-common-project/hadoop-common U: hadoop-common-project/hadoop-common Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/11285/console Powered by Apache Yetus 0.5.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.
          Hide
          jojochuang Wei-Chiu Chuang added a comment -

          +1

          Show
          jojochuang Wei-Chiu Chuang added a comment - +1
          Hide
          jlowe Jason Lowe added a comment -

          +1, patch looks good to me. This is a simpler, more efficient way to test it so I'll replace the trunk version as well. Committing this.

          Show
          jlowe Jason Lowe added a comment - +1, patch looks good to me. This is a simpler, more efficient way to test it so I'll replace the trunk version as well. Committing this.
          Hide
          jlowe Jason Lowe added a comment -

          Thanks for the updated patch and to Wei-Chiu Chuang for additional review! I committed this to trunk and branch-2.

          Show
          jlowe Jason Lowe added a comment - Thanks for the updated patch and to Wei-Chiu Chuang for additional review! I committed this to trunk and branch-2.
          Hide
          hudson Hudson added a comment -

          SUCCESS: Integrated in Jenkins build Hadoop-trunk-Commit #11003 (See https://builds.apache.org/job/Hadoop-trunk-Commit/11003/)
          Revert "HADOOP-13709. Ability to clean up subprocesses spawned by Shell (jlowe: rev 169bfc09037595610eb000fd3a0cb63cc9deca06)

          • (edit) hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/util/Shell.java
          • (edit) hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/util/TestShell.java
            HADOOP-13709. Ability to clean up subprocesses spawned by Shell when the (jlowe: rev 631f1daee3507a1adbc68b937cca31c27dbe8d3d)
          • (edit) hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/util/Shell.java
          • (edit) hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/util/TestShell.java
          Show
          hudson Hudson added a comment - SUCCESS: Integrated in Jenkins build Hadoop-trunk-Commit #11003 (See https://builds.apache.org/job/Hadoop-trunk-Commit/11003/ ) Revert " HADOOP-13709 . Ability to clean up subprocesses spawned by Shell (jlowe: rev 169bfc09037595610eb000fd3a0cb63cc9deca06) (edit) hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/util/Shell.java (edit) hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/util/TestShell.java HADOOP-13709 . Ability to clean up subprocesses spawned by Shell when the (jlowe: rev 631f1daee3507a1adbc68b937cca31c27dbe8d3d) (edit) hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/util/Shell.java (edit) hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/util/TestShell.java

            People

            • Assignee:
              ebadger Eric Badger
              Reporter:
              ebadger Eric Badger
            • Votes:
              0 Vote for this issue
              Watchers:
              9 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development