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