hadoop-yarn-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Sidharta Seethana (JIRA)" <j...@apache.org>
Subject [jira] [Commented] (YARN-2619) NodeManager: Add cgroups support for disk I/O isolation
Date Tue, 28 Apr 2015 04:45:07 GMT

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

Sidharta Seethana commented on YARN-2619:

Thanks for the patch, [~vvasudev]. Overall, the patch seems good - my only concern is about
a dependency from TestCgroupsHandlerImpl to TestCgroupsLCEResourceHandler. More details inline:

h3. YarnConfiguration.java
There seem to be a bunch of unrelated formatting changes in YarnConfiguration.java - consider
removing these changes. 

h3. CGroupsHandler.java

public static final String CGROUP_FILE_TASKS = "tasks”;
public static final String CGROUP_PARAM_CLASSID = "classid";
String CGROUP_PARAM_BLKIO = "weight”;

Make this string “public static final” to be consistent with the rest of the the strings
here? It doesn’t seem like ‘weight’ is the only supported cgroup param for blkio. ,
consider re-naming accordingly ( e.g see classid above ). Alternatively, maybe rename both?

h3. CGroupsHandlerImpl.java
private void init() throws ResourceHandlerException {

Extra empty line before this function.
Map<CGroupController, String> getControllerPaths() {
  return controllerPaths;

Private, modifiable state is being exposed here - use an unmodifiable map.
h3. CGroupsLCEResourceHandler.java
Map<CGroupController, String> getControllerPaths() {
  return controllerPaths;

Same comment as above. Private, modifiable state is being exposed here - use an unmodifiable
h3. Test CGroupsHandlerImpl.java

File parentDir = new File(tmpPath);
// create mock cgroup
File cpuCgroupMountDir =
    TestCgroupsLCEResourcesHandler.createMockCgroupMount(parentDir, "cpu",
File blkioCgroupMountDir =
      "blkio", hierarchy);
File mockMtabFile =
MyCgroupsHandler handler =
    new MyCgroupsHandler(conf, privilegedOperationExecutorMock);

IMO, it doesn’t make sense to have a dependency on TestCgroupsLCEResourceHandler here. Eventually,
we want to get rid of CgroupsLCEResourceHandler (which is very CPU specific) and implement
this using the new resource handler framework. CGroupsHandlerImpl is meant to be a replacement
for (the cgroups functionality of)  CgroupsLCEResourceHandler (See YARN-3542). Given that
this is the case, we should consider implementing this functionality in TestCGroupsHandlerImpl.
This file also has some unrelated formatting changes. 
static void nullifyResourceHandlerChain() throws ResourceHandlerException {
Can this be made private instead of package-private? This shouldn’t be available for use
except for testing.

h3. CGroupsDiskResourceHandlerImpl.java
Class Name : Maybe we should use something less ‘generic’ - (with CFQ?) I expect that
we’ll have more disk/cgroup resource handlers in place in the future - so a less generic
name might be better here. 
String[] lines = data.split("\n”);
Use %n or System.lineSeperator() ?

if (partition.startsWith("sd") || partition.startsWith("hd")) {

Is there a reason to only use these prefixes? I have seen vd* on some virtualized envs, for

PrivilegedOperation.OperationType.ADD_PID_TO_CGROUP, "cgroups="
  + cGroupsHandler.getPathForCGroupTasks(
Use PrivilegedOperation.CGROUP_ARG_PREFIX instead ?

h3. TestCGroupsDiskResourceHandlerImpl.java 
Assert.assertEquals("cgroups=" + path, args.get(0));

Use PrivilegedOperation.CGROUP_ARG_PREFIX instead.

> NodeManager: Add cgroups support for disk I/O isolation
> -------------------------------------------------------
>                 Key: YARN-2619
>                 URL: https://issues.apache.org/jira/browse/YARN-2619
>             Project: Hadoop YARN
>          Issue Type: Sub-task
>            Reporter: Wei Yan
>            Assignee: Wei Yan
>         Attachments: YARN-2619-1.patch, YARN-2619.002.patch

This message was sent by Atlassian JIRA

View raw message