[
https://issues.apache.org/jira/browse/HADOOP-5420?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12708839#action_12708839
]
Vinod K V commented on HADOOP-5420:
-----------------------------------
Patch is coming out good. Some more comments, mostly minor:
main.c
- We should have a separate exit-code for the case when pid!=ppid in
verify_parent() function.
- I don't see the per-command checks for the number of arguments.
atoi(argv[optind++]) will segfault if we go beyond arg-length. Correspondingly,
the null checks and pid<=0 can be done away with in task-controller.c once we
have per-command check for the number of arguments.
- The following code-comment has to be removed. And the check should be only
for 2 args : user and ttroot
{code}
// when we support additional commands without ttroot, this check
// may become command specific.
if (argc < 4) {
display_usage(stderr);
return INVALID_ARGUMENT_NUMBER;
}
{code}
taskcontroller.c
- javadoc for kill_user_task should be "Function used to terminate/kill a task
launched by the user."
taskcontroller.h
- Consistent with the terminology elsewhere, KILL_TASK and DESTROY_TASK_JVM
should instead be TERMINATE_TASK and KILL_TASK
ProcessTree.java
- killProcessGroup(String pid, boolean isProcessGroup) isn't used anywhere,
can be removed.
- Can the the terminate* methods, kill*, destroy*, sigKill* and is*Alive
methods grouped together?
- Need to fix the javadoc of terminateProcess, killProcess,
terminateProcessGroup, killProcessGroup, isAlive() and isProcessGroupAlive()
- sigKillInCurrentThread: The following code is misplaced, it should be inside
the "if (isProcessGroup || ProcessTree.isAlive(pid))" check (+68).
{code}
if(isProcessGroup) {
killProcessGroup(pid);
} else {
killProcess(pid);
}
{code}
TaskController.java
- Rename killTaskJvm to cleanupTaskJvm ??
- Javadoc of this method should instead read "Sends a kill signal to
forcefully kill the task JVM and all its sub-processes"
DefaultTaskController.java
- killTask(): put a comment for the WINDOWS case
- killTask(): "else" not needed
LinuxTaskController.java
- Consistent with the terminology elsewhere, KILL_TASK_JVM and
DESTROY_TASK_JVM should be TERMINATE_TASK_JVM and KILL_TASK_JVM
- TT is not brought down when it fails to write pid on all the configured
local directories.
- Log statement should be added so that we know on which directories TT could
write the pid file.
- When on some directory, pid files are not created because of a transient
errors, tasks launched on that directory will not run. Should we read pid from
other disks?
TestKillSubProcessesWithLinuxTaskController
- Remove blank lines
TestKillSubProcesses:
- In runJob, why is the check needed for the inDir and outDir?
- In isAlive(), if we get an IOException while running the ps command, we need
to re-throw this exception instead of just logging it.
> Support killing of process groups in LinuxTaskController binary
> ---------------------------------------------------------------
>
> Key: HADOOP-5420
> URL: https://issues.apache.org/jira/browse/HADOOP-5420
> Project: Hadoop Core
> Issue Type: Bug
> Reporter: Sreekanth Ramakrishnan
> Assignee: Sreekanth Ramakrishnan
> Attachments: hadoop-5420-1.patch, hadoop-5420-2.patch,
> hadoop-5420-3.patch, hadoop-5420-4.patch, hadoop-5420.patch
>
>
> Support setsid based kill in LinuxTaskController.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
