hadoop-yarn-issues mailing list archives

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

     [ https://issues.apache.org/jira/browse/YARN-2619?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]

Varun Vasudev updated YARN-2619:
--------------------------------
    Attachment: YARN-2619.003.patch

{quote}
YarnConfiguration.java
There seem to be a bunch of unrelated formatting changes in YarnConfiguration.java - consider
removing these changes.
CGroupsHandler.java
{quote}

It fixes some checkstyle warnings about lines being more than 80 characters in length. Let
me know if you feel strongly about it and I'll revert them.

{quote}
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 CGROUP_PARAM_BLKIO_WEIGHT.
{quote}

Changed to public static final and renamed to CGROUP_PARAM_BLKIO_WEIGHT.

{quote}
CGroupsHandlerImpl.java

private void init() throws ResourceHandlerException {
  initializeControllerPaths();
}

Extra empty line before this function.
{quote}

Fixed.

{quote}
\@VisibleForTesting
Map<CGroupController, String> getControllerPaths() {
  return controllerPaths;
}

Private, modifiable state is being exposed here - use an unmodifiable map.
CGroupsLCEResourceHandler.java

@VisibleForTesting
Map<CGroupController, String> getControllerPaths() {
  return controllerPaths;
}

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

{quote}

Fixed both.

{quote}
Test CGroupsHandlerImpl.java

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

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.

{quote}

Good point. Moved the functions to TestCGroupsHandlerImpl. The formatting changes are similar
to the ones in YarnConfiguration - fixing some checkstyle warnings.

{quote}
@VisibleForTesting
static void nullifyResourceHandlerChain() throws ResourceHandlerException {

Can this be made private instead of package-private? This shouldn’t be available for use
except for testing.

{quote}

If it's made private, the tests can't reset the resource handler chain. The annotation isn't
loaded into the JVM. If you have any other way to reset the chain, let me know.

{quote}
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.
{quote}

Changed it to CGroupsBlkioResourceHandler. Is that ok?

{quote}

String[] lines = data.split("\n”);

Use %n or System.lineSeperator() ?
{quote}

Fixed.

{quote}
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.

{quote}

Added vd and xvd prefix checks.

{quote}
PrivilegedOperation.OperationType.ADD_PID_TO_CGROUP, "cgroups="
  + cGroupsHandler.getPathForCGroupTasks(

Use PrivilegedOperation.CGROUP_ARG_PREFIX instead ?
TestCGroupsDiskResourceHandlerImpl.java

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

Use PrivilegedOperation.CGROUP_ARG_PREFIX instead.
{quote}

Fixed.

> 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, YARN-2619.003.patch
>
>




--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

Mime
View raw message