[ https://issues.apache.org/jira/browse/YARN-9098?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16767298#comment-16767298 ]
Peter Bacsko commented on YARN-9098: ------------------------------------ Maybe it's just nitpicking, but... {noformat} public List<String> getPathsForController(String controller) { return mappings.entrySet().stream() .filter(e -> e.getValue().contains(controller)) .map(Map.Entry::getKey) .collect(Collectors.toList()); } {noformat} Is it ok to use {{contains()}} here? If cpu and cpuacct are mounted to two different directories, then we might return wrong path for cpu, no? Usually they're mounted to the same directory like {{/sys/fs/cgroup/cpu,cpuacct}} but it's something to think about. > Separate mtab file reader code and cgroups file system hierarchy parser code > from CGroupsHandlerImpl and ResourceHandlerModule > ------------------------------------------------------------------------------------------------------------------------------ > > Key: YARN-9098 > URL: https://issues.apache.org/jira/browse/YARN-9098 > Project: Hadoop YARN > Issue Type: Improvement > Reporter: Szilard Nemeth > Assignee: Szilard Nemeth > Priority: Major > Attachments: YARN-9098.002.patch, YARN-9098.003.patch, > YARN-9098.004.patch, YARN-9098.005.patch, YARN-9098.006.patch > > > Separate mtab file reader code and cgroups file system hierarchy parser code > from CGroupsHandlerImpl and ResourceHandlerModule > CGroupsHandlerImpl has a method parseMtab that parses an mtab file and stores > cgroups data. > CGroupsLCEResourcesHandler also has a method with the same name, with > identical code. > The parser code should be extracted from these places and be added in a new > class as this is a separate responsibility. > As the output of the file parser is a Map<String, Set<String>>, it's better > to encapsulate it in a domain object, named 'CGroupsMountConfig' for instance. > ResourceHandlerModule has a method named parseConfiguredCGroupPath, that is > responsible for producing the same results (Map<String, Set<String>>) to > store cgroups data, it does not operate on mtab file, but looking at the > filesystem for cgroup settings. As the output is the same, CGroupsMountConfig > should be used here, too. > Again, this could should not be part of ResourceHandlerModule as it is a > different responsibility. > One more thing which is strongly related to the methods above is > CGroupsHandlerImpl.initializeFromMountConfig: This method processes the > result of a parsed mtab file or a parsed cgroups filesystem data and stores > file system paths for all available controllers. This method invokes > findControllerPathInMountConfig, which is a duplicated in CGroupsHandlerImpl > and CGroupsLCEResourcesHandler, so it should be moved to a single place. To > store filesystem path and controller mappings, a new domain object could be > introduced. -- This message was sent by Atlassian JIRA (v7.6.3#76005) --------------------------------------------------------------------- To unsubscribe, e-mail: yarn-issues-unsubscr...@hadoop.apache.org For additional commands, e-mail: yarn-issues-h...@hadoop.apache.org