[jira] [Commented] (YARN-3542) Re-factor support for CPU as a resource using the new ResourceHandler mechanism

[Sidharta Seethana (JIRA)]

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

Sidharta Seethana commented on YARN-3542:
-----------------------------------------

hi [~vvasudev], thanks for the updated patch. Please see comments below.

h4. CGroupsCpuResourceHandlerImpl.java

{code}
import 
org.apache.hadoop.yarn.server.nodemanager.util.CgroupsLCEResourcesHandler;
import 
org.apache.hadoop.yarn.server.nodemanager.util.DefaultLCEResourcesHandler;
import org.apache.hadoop.yarn.server.nodemanager.util.LCEResourcesHandler;
{code}
These are unused imports in CGroupsCpuResourceHandlerImpl

{code}
@VisibleForTesting
static final String CPU_PERIOD_US = "cfs_period_us";
@VisibleForTesting
static final String CPU_QUOTA_US = "cfs_quota_us";
@VisibleForTesting
static final String CPU_SHARES = "shares”;
{code}

Move these to CGroupsHandler ?

{code}
    int quotaUS = MAX_QUOTA_US;
    int periodUS = (int) (MAX_QUOTA_US / yarnProcessors);
{code}

About how shares/cfs_period_us/cfs_quota_us are used : additional 
comments/documentation and examples (as unit tests?) would be useful. It took 
me a while to trace through the code using some examples. 

h4. CGroupsLCEResourcesHandler.java

Since the class has been marked deprecated, is it necessary to make the rest of 
the changes that are included ?

h4. LinuxContainerExecutor.java
{code}
private LCEResourcesHandler getResourcesHandler(Configuration conf) {
  LCEResourcesHandler handler = ReflectionUtils.newInstance(
      conf.getClass(YarnConfiguration.NM_LINUX_CONTAINER_RESOURCES_HANDLER,
          DefaultLCEResourcesHandler.class, LCEResourcesHandler.class), conf);

  // Stop using CgroupsLCEResourcesHandler
  // use the resource handler chain instead
  // ResourceHandlerModule will create the cgroup cpu module if
  // CgroupsLCEResourcesHandler is set
  if (handler instanceof CgroupsLCEResourcesHandler) {
    handler =
        ReflectionUtils.newInstance(DefaultLCEResourcesHandler.class, conf);
  }
  handler.setConf(conf);
  return handler;
}
{code}

Since all resource handling is now in the resource handler chain - there is no 
longer a need to have references to LCEResourcesHandler in 
LinuxContainerExeuctor - all related config handling etc should be in 
ResourceHandlerModule.java (which already seems to the case). IMO, all 
references to LCEResourcesHandler (and sub-classes) should be removed from 
LinuxContainerExecutor. 

> Re-factor support for CPU as a resource using the new ResourceHandler 
> mechanism
> -------------------------------------------------------------------------------
>
>                 Key: YARN-3542
>                 URL: https://issues.apache.org/jira/browse/YARN-3542
>             Project: Hadoop YARN
>          Issue Type: Bug
>          Components: nodemanager
>            Reporter: Sidharta Seethana
>            Assignee: Sidharta Seethana
>            Priority: Critical
>         Attachments: YARN-3542.001.patch, YARN-3542.002.patch, 
> YARN-3542.003.patch
>
>
> In YARN-3443 , a new ResourceHandler mechanism was added which enabled easier 
> addition of new resource types in the nodemanager (this was used for network 
> as a resource - See YARN-2140 ). We should refactor the existing CPU 
> implementation ( LinuxContainerExecutor/CgroupsLCEResourcesHandler ) using 
> the new ResourceHandler mechanism. 



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