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:
+        if (LOG.isWarnEnabled()) {
+          LOG.warn("Invalid argument: " + arg);
+        }
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.

+@InterfaceStability.Unstable class CGroupsHandlerImpl
+    implements CGroupsHandler {
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)
+  List<PrivilegedOperation> teardown() throws ResourceHandlerException;
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

Reply via email to