[ 
https://issues.apache.org/jira/browse/YARN-639?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13682914#comment-13682914
 ] 

Omkar Vinit Joshi commented on YARN-639:
----------------------------------------

[~zjshen] looked at your patch... minor comments

bq. +    nodeManager.stop();
* in AM, can you rename nodemanager to something like nmClient? It was 
confusing when I saw nodemanager.stop() :).
{code}
+    @Override
+    public void onContainerStarted(ContainerId containerId,
+        Map<String, ByteBuffer> allServiceResponse) {
+      LOG.info("Succeeded to start Container " + containerId);
+      Container container = containers.get(containerId);
+      if (container != null) {
+        nodeManager.getContainerStatus(containerId, container.getNodeId(),
+            container.getContainerToken());
+      }
+    }
{code}
* why do we need to do getContainerStatus after successfully starting it? is it 
required?

* also can you change LOG.info in CallBackHandler to LOG.debug / LOG.error as 
appropriate?

everything else looks good.. 
                
> Make AM of Distributed Shell Use NMClient
> -----------------------------------------
>
>                 Key: YARN-639
>                 URL: https://issues.apache.org/jira/browse/YARN-639
>             Project: Hadoop YARN
>          Issue Type: Bug
>            Reporter: Zhijie Shen
>            Assignee: Zhijie Shen
>         Attachments: YARN-639.1.patch
>
>
> YARN-422 adds NMClient. AM of Distributed Shell should use it instead of 
> using ContainerManager directly.

--
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

Reply via email to