[
https://issues.apache.org/jira/browse/YARN-233?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13501864#comment-13501864
]
Bikas Saha commented on YARN-233:
---------------------------------
Overral looks good. Nice work on the shell script builder.
Didnt quite get this removal?
{code}
+++
hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-app/src/main/java/org/apache/hadoop/mapred/MapReduceChildJVM.java
...
- vargs.add("exec");
{code}
ContainerExecutor.java
Should this not true? Jobobject in winutils task are doing what setsid does for
process groups. setsid is a bad name because it indicates the implementation
rather than the concept of process groups. In branch-1 its used to determine
whether tasks should be killed using group methods or not etc. So setting it to
false might disable other functionality that is based on process groups.
MAPREDUCE-4325 tracks renaming setsid but it did not make sense to do it on
branch-1. On trunk we should rename it. Perhaps move MAPREDUCE-4325 to YARN and
do it.
Alternatively, if you think setsid does not have the group concept in trunk
then we should use setsid only under Linux (ie regardless of Windows) and also
rename it to setSidLinux or something.
{code}
private static boolean isSetsidSupported() {
+ if (Shell.WINDOWS) {
+ return false;
+ }
{code}
DefaultContainerExecutor.java
If possible, it would be preferable to minimize if(Shell.WINDOWS) in the code
by moving it inside Shell.java so that its all in one place.
{code}
+ LocalWrapperScriptBuilder sb = Shell.WINDOWS ?
+ new WindowsLocalWrapperScriptBuilder(containerIdStr, containerWorkDir) :
+ new UnixLocalWrapperScriptBuilder(containerWorkDir);
{code}
{code}
ContainerLaunch.java
+ public static final String CONTAINER_SCRIPT = Shell.WINDOWS ?
+ "launch_container.cmd" : "launch_container.sh";
{code}
Good place to create/use a Shell.getKillCommand(). I think branch-1-win already
has it.
{code}
+ } else {
+ ShellCommandExecutor shexec = new ShellCommandExecutor(new String[] {
+ Shell.WINUTILS, "task", "kill", pid });
+ shexec.execute();
+ }
+ } else {
+ ShellCommandExecutor shexec = new ShellCommandExecutor(new String[] {
"kill",
+ "-" + signal.getValue(), pid });
+ shexec.execute();
+ }
{code}
ProcessIdFileReader.java
Instead of removing the code we could scope it for linux. Also for windows
would be a helpful sanity check to use ConverterUtils to check that the string
is a valid container id.
{code}
- try {
- Long pid = Long.valueOf(temp);
- if (pid > 0) {
- processId = temp;
- break;
- }
- } catch (Exception e) {
- // do nothing
- }
+ processId = temp;
+ break;
{code}
It would be great if we can check that when a container is spawned by the NM
then the first process created in the tree is WINUTILS.exe
> YARN support for execution of container on Windows
> --------------------------------------------------
>
> Key: YARN-233
> URL: https://issues.apache.org/jira/browse/YARN-233
> Project: Hadoop YARN
> Issue Type: Bug
> Components: nodemanager
> Affects Versions: trunk-win
> Reporter: Chris Nauroth
> Assignee: Chris Nauroth
> Attachments: YARN-233-branch-trunk-win.patch
>
>
> YARN container launch code requires updates for compatibility with Windows.
> Instead of launching and killing tasks via generated bash scripts and kill,
> use generated cmd scripts and winutils for task management.
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators
For more information on JIRA, see: http://www.atlassian.com/software/jira
