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

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

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 CGROUP_PARAM_BLKIO_WEIGHT.

h3. CGroupsHandlerImpl.java
{code}
private void init() throws ResourceHandlerException {
  initializeControllerPaths();
}
{code}

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

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

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

{code}
File parentDir = new File(tmpPath);
// create mock cgroup
File cpuCgroupMountDir =
    TestCgroupsLCEResourcesHandler.createMockCgroupMount(parentDir, "cpu",
      hierarchy);
Assert.assertTrue(cpuCgroupMountDir.exists());
File blkioCgroupMountDir =
    TestCgroupsLCEResourcesHandler.createMockCgroupMount(parentDir,
      "blkio", hierarchy);
Assert.assertTrue(blkioCgroupMountDir.exists());
File mockMtabFile =
    TestCgroupsLCEResourcesHandler.createMockMTab(parentDir);
MyCgroupsHandler handler =
    new MyCgroupsHandler(conf, privilegedOperationExecutorMock);
{code}

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. 
{code}
@VisibleForTesting
static void nullifyResourceHandlerChain() throws ResourceHandlerException {
{code}
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. 
{code}
String[] lines = data.split("\n”);
{code}
Use %n or System.lineSeperator() ?

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

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

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

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

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
(v6.3.4#6332)

Mime
View raw message