[jira] [Commented] (YARN-3443) Create a 'ResourceHandler' subsystem to ease addition of support for new resource types on the NM

[Varun Vasudev (JIRA)]

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

Varun Vasudev commented on YARN-3443:
-------------------------------------

Thanks for uploading the patch [~sidharta-s]!

Some comments 

# In PrivilegedOperationExecutor.java
{noformat}
catch (ExitCodeException e) {
  if (LOG.isWarnEnabled()) {
     LOG.warn("Shell execution returned exit code: " + exec.getExitCode());
     LOG.warn("Privileged Execution Operation Output:");
     LOG.warn(exec.getOutput());
  }
}
{noformat}
Can you merge the warnings into one warn message and add the exception to the 
log message? This is so that alerts based on the number of errors/warnings 
reflect the actual number.
# In CGroupHandlerImpl.java
{noformat}
if (enableCGroupMount) {
    //nothing to do here - we support 'deferred' mounting of specific
    //controllers - we'll populate the path for a given controller when an
    //explicit mountCGroupController request is issued.
} else {
    //cluster admins are expected to have mounted controllers in specific
    //locations - we'll attempt to figure out mount points
    initializeControllerPathsFromMtab();
}
{noformat}
Can we remove the empty if block?
# In CGroupHandlerImpl.java
{noformat}
LOG.error("Mount point Based on mtab file : " + MTAB_FILE);
LOG.error("Controller mount point not writable for: " + name);
{noformat}
Merge the log messages
# In CGroupHandlerImpl.java
{noformat}
LOG.warn("Failed to initialize controller paths!");
LOG.warn(e);
{noformat}
Merge the log messages
# In CGroupHandlerImpl.java
In initializeControllerPathsFromMtab, when the error and warnings are logged, 
should we throw an exception, since the functionality being requested will not 
work?
# In CGroupHandlerImpl.java
In mountCGroupController
{noformat}
if (!enableCGroupMount) {
    if (LOG.isWarnEnabled()) {
      LOG.warn("CGroup mounting is disabled - ignoring mount request for: " +
            controller.getName());
      return;
    }
}
{noformat}
Should this be an error with an exception being thrown since the functionality 
being requested won't be possible?
# In CGroupHandlerImpl.java
{noformat}
@param file object referring to the cgroup to be deleted mislabelled
{noformat}
The param name is mislabeled. It should be cgf instead of file
# In ResourceHandler.java
{noformat}
   * @param containerId if of the container being reacquired.
   * @return
   * @throws ResourceHandlerException
{noformat}
return is missing description
# In ResourceHandler.java
{noformat}
List<PrivilegedOperation> postComplete(ContainerId containerId) throws
      ResourceHandlerException;

;
{noformat}
There's an extra semi-colon.
# In TestPrivilegedOperationExecutor.java
{noformat}
expectedPath = customExecutorPath;
containerExePath = PrivilegedOperationExecutor
        .getContainerExecutorExecutablePath(confWithExecutorPath);
Assert.assertEquals(expectedPath, customExecutorPath);
{noformat}
containerExePath is not used. The assert will always succeed since you set them 
to be equal before calling the function. Did you mean to compare 
containerExePath and expectedPath?
# In TestPrivilegedOperationExecutor.java
{noformat}
prefixLength + 0
{noformat}
Remove the 0?
# In TestPrivilegedOperationExecutor.java
{noformat}
try {
      PrivilegedOperationExecutor.squashCGroupOperations(ops);
} catch (PrivilegedOperationException e) {
      if (LOG.isInfoEnabled()) {
        LOG.info("Caught expected exception : " + e);
      }
      caughtPrivilegedOperationException = true;
}

Assert.assertTrue("Expected squash operation to fail!",
        caughtPrivilegedOperationException);
{noformat}
Instead of using the boolean to check if the exception was thrown, you can use 
Assert.fail in the try block. This pattern occurs multiple times.
# I think the formatting is a bit off in a couple of files. Can you apply the 
formatter and check?

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