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