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? CGROUP_PARAM_NET_CLS_CLASSID and 

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 map.
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 example. 

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

Reply via email to