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

Junping Du commented on YARN-3443:
----------------------------------

Thanks [~sidharta-s] for delivering the patch and [~vvasudev] for reviewing. 
Just quickly walk though the patch, the patch looks good in overall. Some minor 
comments below, but more comments may come later:
{code}
+        if (LOG.isWarnEnabled()) {
+          LOG.warn("Invalid argument: " + arg);
+        }
{code}
We typically log message directly on INFO or above level (like: warn, error), 
not necessary for additional check for LOG.isWarnEnabled(). Also there are some 
other places for checking LOG.isWarnEnabled() or even LOG.isErrorEnabled() 
which should be unnecessary.

{code}
+@InterfaceAudience.Private
+@InterfaceStability.Unstable class CGroupsHandlerImpl
+    implements CGroupsHandler {
{code}
We should separate annotation from the line of calss definition. Also, it 
should be a public class?

In ResourceHandler.java (and other places, like: ResourceHandlerChain)
{code}
+  List<PrivilegedOperation> teardown() throws ResourceHandlerException;
{code}
teardown => tearDown


> Create a 'ResourceHandler' subsystem to ease addition of support for new 
> resource types on the NM
> -------------------------------------------------------------------------------------------------
>
>                 Key: YARN-3443
>                 URL: https://issues.apache.org/jira/browse/YARN-3443
>             Project: Hadoop YARN
>          Issue Type: Sub-task
>          Components: nodemanager
>            Reporter: Sidharta Seethana
>            Assignee: Sidharta Seethana
>         Attachments: YARN-3443.001.patch, YARN-3443.002.patch, 
> YARN-3443.003.patch, YARN-3443.004.patch
>
>
> The current cgroups implementation is closely tied to supporting CPU as a 
> resource . We need to separate out CGroups support as well a provide a simple 
> ResourceHandler subsystem that will enable us to add support for new resource 
> types on the NM - e.g Network, Disk etc. 



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

Reply via email to