Hadoop Common
  1. Hadoop Common
  2. HADOOP-5488

HADOOP-2721 doesn't clean up descendant processes of a jvm that exits cleanly after running a task successfully

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 0.21.0
    • Component/s: None
    • Labels:
      None
    • Hadoop Flags:
      Reviewed
    1. hadoop-5488-ydist.patch
      78 kB
      Sreekanth Ramakrishnan
    2. HADOOP_5488_5614.v1.3.patch
      60 kB
      Ravi Gummadi
    3. HADOOP_5488_5614.v1.2.patch
      59 kB
      Ravi Gummadi
    4. HADOOP_5488_5614.v1.patch
      60 kB
      Ravi Gummadi
    5. testcase5488.patch
      17 kB
      Ravi Gummadi
    6. HADOOP_5488_5614.patch
      57 kB
      Ravi Gummadi
    7. HADOOP-5488.patch
      49 kB
      Ravi Gummadi

      Issue Links

        Activity

        Hide
        Vinod Kumar Vavilapalli added a comment -

        HADOOP-2721 incorporated changes to clean up descendant processes only in the code path of killing of a task. And when a jvm that successfully ran a task exits, the pidfile gets removed and so even if a KillTaskAction is subsequently received, descendants will not be cleaned up for the lack of the root pid.

        Show
        Vinod Kumar Vavilapalli added a comment - HADOOP-2721 incorporated changes to clean up descendant processes only in the code path of killing of a task. And when a jvm that successfully ran a task exits, the pidfile gets removed and so even if a KillTaskAction is subsequently received, descendants will not be cleaned up for the lack of the root pid.
        Hide
        Devaraj Das added a comment -

        In addition to successful tasks, tasks that fail should also be taken care of.

        Show
        Devaraj Das added a comment - In addition to successful tasks, tasks that fail should also be taken care of.
        Hide
        Ravi Gummadi added a comment -

        For successful tasks and failed tasks, as JvmRunner.kill() is not getting called, their subprocesses are not killed. Even if we call kill for these tasks, the cleanup of pidFile might be done already and we can't get pid of taskJvm.

        So I am planning to export pid of task in some env variable and then sending pid of taskJvm from Child(task) to parent(TT) using umbilical protocol ---- changing the methodCall umbilical.getTask(JVMId) to umbilical.getTask(JVMId, pid) in Child.java and getTask() in TT would set pid in jvmIdToPidMap, which will be used by JvmRunner.kill(). And JvmRunner.runChild() will call kill() in the finally block, if it was not called earlier for this jvm.
        This approach would avoid the pidFiles altogether.

        Thoughts ?

        Show
        Ravi Gummadi added a comment - For successful tasks and failed tasks, as JvmRunner.kill() is not getting called, their subprocesses are not killed. Even if we call kill for these tasks, the cleanup of pidFile might be done already and we can't get pid of taskJvm. So I am planning to export pid of task in some env variable and then sending pid of taskJvm from Child(task) to parent(TT) using umbilical protocol ---- changing the methodCall umbilical.getTask(JVMId) to umbilical.getTask(JVMId, pid) in Child.java and getTask() in TT would set pid in jvmIdToPidMap, which will be used by JvmRunner.kill(). And JvmRunner.runChild() will call kill() in the finally block, if it was not called earlier for this jvm. This approach would avoid the pidFiles altogether. Thoughts ?
        Hide
        Hemanth Yamijala added a comment -

        changing the methodCall umbilical.getTask(JVMId) to umbilical.getTask(JVMId, pid) in Child.java

        It seems cleaner to define a new method: umbilical.setProcessHandle(JVMId, handle). This way we send this information only once per JVM. We can also handle cases like what happens if this call fails. For e.g., I suppose this call will be made before the child runs the task, and if this call fails, it should ideally abort.

        Show
        Hemanth Yamijala added a comment - changing the methodCall umbilical.getTask(JVMId) to umbilical.getTask(JVMId, pid) in Child.java It seems cleaner to define a new method: umbilical.setProcessHandle(JVMId, handle). This way we send this information only once per JVM. We can also handle cases like what happens if this call fails. For e.g., I suppose this call will be made before the child runs the task, and if this call fails, it should ideally abort.
        Hide
        Ravi Gummadi added a comment -

        Attaching patch with code changes for killing subprocesses of successful tasks and failed tasks. Usage of pid files is removed and pid is exported to an env variable and then sent from taskJvm to TT using umbilical protocol, which will be used for killing subprocesses of tasks.

        Please review and provide your comments.

        Show
        Ravi Gummadi added a comment - Attaching patch with code changes for killing subprocesses of successful tasks and failed tasks. Usage of pid files is removed and pid is exported to an env variable and then sent from taskJvm to TT using umbilical protocol, which will be used for killing subprocesses of tasks. Please review and provide your comments.
        Hide
        Vinod Kumar Vavilapalli added a comment -

        This patch clashes with HADOOP-4490 which is almost close to commit. This patch should wait for HADOOP-4490 and the changes should be merged.

        Show
        Vinod Kumar Vavilapalli added a comment - This patch clashes with HADOOP-4490 which is almost close to commit. This patch should wait for HADOOP-4490 and the changes should be merged.
        Hide
        Devaraj Das added a comment -

        Sorry for not looking at the patch earlier. But I think the RPC getTask(JVMId) could simply be enhanced to take a (new class object) JVMContext, i.e., getTask(JVMContext). The class could have the fields as PID and the JVMId, and in the case of windows, the PID string is empty.

        Show
        Devaraj Das added a comment - Sorry for not looking at the patch earlier. But I think the RPC getTask(JVMId) could simply be enhanced to take a (new class object) JVMContext, i.e., getTask(JVMContext). The class could have the fields as PID and the JVMId, and in the case of windows, the PID string is empty.
        Hide
        Ravi Gummadi added a comment -

        Incorporated Devaraj's comment.

        Attaching combined patch of 5488 and 5614 as there are some clashing code changes.

        Please review and provide your comments.

        Show
        Ravi Gummadi added a comment - Incorporated Devaraj's comment. Attaching combined patch of 5488 and 5614 as there are some clashing code changes. Please review and provide your comments.
        Hide
        Vinod Kumar Vavilapalli added a comment -

        Gone through the patch. Looks good overall. Some code comments:

        • JvmContext : Constructor should just take a pid and child jvm should pass it after reading it from the environment.
        • The 'isProcessGroup' check is not needed in ProcessTree.sigKillInCurrentThread() as we may want to send SIGKILL to processes not started as sessions also.
        • Can you please remove the unused import in DefaultTaskController that was perhaps mistakenly left in HADOOP-4490:
          import org.apache.hadoop.fs.Path;
        • No need for changing the controller.killTaskJVM() to return a boolean. If jvmmanager.kill() has to be reentrant it can just maintain the variables.
          • Is it needed to make kill to be reentrant now?
        • I think DefaultTaskController.killTaskJvm() method should be better commented, especially the various conditionals.
        • Fix the LOG var of the abstract class Task (HADOOP-5351)
        • TaskLog: The place where the comment says pid files are not used anymore, we shoujld also suggest the method that should instead be used directly.
        • There should be a comment about the bump up of the version number of TaskUmbilicalProtocol saying there is a change in signature of getTask()
        • TestKillSubProcesses:
          • Most of the methods in this class can be private instead of package-private unless explicitly needed.
          • I see some hardcoded values 5 in the code like in "scriptDir.toString().substring(5)". This is perhaps related to parsing out the filesystem scheme info. We should instead use URI methods to get the actual path name.
          • KillingMapperWithChildren, FailingMapperWithChildren and SucceedMapperWithChildren can be futher refactored.
          • Can we use some job in UtilsForTest instead of our own runJob() ?
        • Is this possible to test if this test-case fails without the changes in the patch and succeeds with the changes?
        Show
        Vinod Kumar Vavilapalli added a comment - Gone through the patch. Looks good overall. Some code comments: JvmContext : Constructor should just take a pid and child jvm should pass it after reading it from the environment. The 'isProcessGroup' check is not needed in ProcessTree.sigKillInCurrentThread() as we may want to send SIGKILL to processes not started as sessions also. Can you please remove the unused import in DefaultTaskController that was perhaps mistakenly left in HADOOP-4490 : import org.apache.hadoop.fs.Path; No need for changing the controller.killTaskJVM() to return a boolean. If jvmmanager.kill() has to be reentrant it can just maintain the variables. Is it needed to make kill to be reentrant now? I think DefaultTaskController.killTaskJvm() method should be better commented, especially the various conditionals. Fix the LOG var of the abstract class Task ( HADOOP-5351 ) TaskLog: The place where the comment says pid files are not used anymore, we shoujld also suggest the method that should instead be used directly. There should be a comment about the bump up of the version number of TaskUmbilicalProtocol saying there is a change in signature of getTask() TestKillSubProcesses: Most of the methods in this class can be private instead of package-private unless explicitly needed. I see some hardcoded values 5 in the code like in "scriptDir.toString().substring(5)". This is perhaps related to parsing out the filesystem scheme info. We should instead use URI methods to get the actual path name. KillingMapperWithChildren, FailingMapperWithChildren and SucceedMapperWithChildren can be futher refactored. Can we use some job in UtilsForTest instead of our own runJob() ? Is this possible to test if this test-case fails without the changes in the patch and succeeds with the changes?
        Hide
        Vinod Kumar Vavilapalli added a comment -

        This patch only addresses the clean-up of descendant processes w.r.t DefaultTaskController. Changes for LinuxTaskController need more effort and will be done as part of HADOOP-5420. Linking the two issues.

        Show
        Vinod Kumar Vavilapalli added a comment - This patch only addresses the clean-up of descendant processes w.r.t DefaultTaskController. Changes for LinuxTaskController need more effort and will be done as part of HADOOP-5420 . Linking the two issues.
        Hide
        Ravi Gummadi added a comment -

        Attaching testcase that fails with trunk because of subprocesses not getting killed for successful and failing tasks.

        Show
        Ravi Gummadi added a comment - Attaching testcase that fails with trunk because of subprocesses not getting killed for successful and failing tasks.
        Hide
        Ravi Gummadi added a comment -

        Attaching patch with the code changes suggested by Vinod.

        Please review and provide your comments.

        Show
        Ravi Gummadi added a comment - Attaching patch with the code changes suggested by Vinod. Please review and provide your comments.
        Hide
        Ravi Gummadi added a comment -

        Incorpoated Vinod's offline comments and syncing with the trunk, attaching new patch.

        Please review and provide your comments.

        Show
        Ravi Gummadi added a comment - Incorpoated Vinod's offline comments and syncing with the trunk, attaching new patch. Please review and provide your comments.
        Hide
        Ravi Gummadi added a comment -

        Attaching new patch with some javadoc change.

        Unit tests passed on my linux machine. No new failures in unit tests on Windows machine.

        Reliability test passed.

        ant test-patch gave:

        [exec] +1 overall.
        [exec]
        [exec] +1 @author. The patch does not contain any @author tags.
        [exec]
        [exec] +1 tests included. The patch appears to include 12 new or modified tests.
        [exec]
        [exec] +1 javadoc. The javadoc tool did not generate any warning messages.
        [exec]
        [exec] +1 javac. The applied patch does not increase the total number of javac compiler warnings.
        [exec]
        [exec] +1 findbugs. The patch does not introduce any new Findbugs warnings.
        [exec]
        [exec] +1 Eclipse classpath. The patch retains Eclipse classpath integrity.
        [exec]
        [exec] +1 release audit. The applied patch does not increase the total number of release audit warnings.

        Show
        Ravi Gummadi added a comment - Attaching new patch with some javadoc change. Unit tests passed on my linux machine. No new failures in unit tests on Windows machine. Reliability test passed. ant test-patch gave: [exec] +1 overall. [exec] [exec] +1 @author. The patch does not contain any @author tags. [exec] [exec] +1 tests included. The patch appears to include 12 new or modified tests. [exec] [exec] +1 javadoc. The javadoc tool did not generate any warning messages. [exec] [exec] +1 javac. The applied patch does not increase the total number of javac compiler warnings. [exec] [exec] +1 findbugs. The patch does not introduce any new Findbugs warnings. [exec] [exec] +1 Eclipse classpath. The patch retains Eclipse classpath integrity. [exec] [exec] +1 release audit. The applied patch does not increase the total number of release audit warnings.
        Hide
        Vinod Kumar Vavilapalli added a comment -

        +1 for the patch.

        Show
        Vinod Kumar Vavilapalli added a comment - +1 for the patch.
        Hide
        Devaraj Das added a comment -

        I just committed this. Thanks, Ravi!

        Show
        Devaraj Das added a comment - I just committed this. Thanks, Ravi!
        Hide
        Hudson added a comment -
        Show
        Hudson added a comment - Integrated in Hadoop-trunk #815 (See http://hudson.zones.apache.org/hudson/job/Hadoop-trunk/815/ )
        Hide
        Sreekanth Ramakrishnan added a comment -

        Attaching patch for Yahoo distribution.

        Show
        Sreekanth Ramakrishnan added a comment - Attaching patch for Yahoo distribution.
        Hide
        Robert Chansler added a comment -

        Editorial pass over all release notes prior to publication of 0.21. Bug.

        Show
        Robert Chansler added a comment - Editorial pass over all release notes prior to publication of 0.21. Bug.

          People

          • Assignee:
            Ravi Gummadi
            Reporter:
            Vinod Kumar Vavilapalli
          • Votes:
            0 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development