----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/51031/#review146028 -----------------------------------------------------------
Mind adding a test? We already have `CgroupsAnyHierarchyTest.ROOT_CGROUPS_Get` but we can add another `ROOT_CGROUPS_Nonresursive_Get` and change the current one into a `ROOT_CGROUPS_Recursive_Get`. The argument `bool recursive` defaults to true today because it's the current behavior. However I am not sure if we actually want this as the default. This is another disucssion to have but as least let's make the tests explicit, i.e., `ROOT_CGROUPS_Nonresursive_Get` and `ROOT_CGROUPS_Recursive_Get` for now. src/linux/cgroups.cpp (lines 914 - 915) <https://reviews.apache.org/r/51031/#comment212384> I feel like we can keep the original comments. It explains the information relevant to how we use fts in one place. The block is short so I wouldn't worry about comments being too far away from the relavent code that hurts readability. Keeping it in one place is more coherent. Perhaps we can remove `The traversal root itself is numbered 0.` because we don't use this special number anymore. e.g., ``` // Use post-order walk here. fts_level is the depth of the traversal, // numbered from -1 to N, where the file/dir was found. fts_info // includes flags for the current node. FTS_DP indicates a directory // being visited in postorder. ``` src/linux/cgroups.cpp (lines 915 - 932) <https://reviews.apache.org/r/51031/#comment212385> We can keep the original if condition (which is simpler) now that we do `fts_set()` right? i.e., how about the following: ``` // Skip traversing the subtree if not recursive. if (!recursive && node->fts_level > FTS_ROOTLEVEL) { fts_set(tree, node, FTS_SKIP); } if (node->fts_level > FTS_ROOTLEVEL && node->fts_info & FTS_DP) { string path = strings::trim(node->fts_path + hierarchyAbsPath.get().length(), "/"); cgroups.push_back(path); } ``` Make sense? - Jiang Yan Xu On Aug. 17, 2016, 10:38 a.m., haosdent huang wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/51031/ > ----------------------------------------------------------- > > (Updated Aug. 17, 2016, 10:38 a.m.) > > > Review request for mesos, Gilbert Song, Jie Yu, Qian Zhang, and Zhengju Sha. > > > Bugs: MESOS-6035 > https://issues.apache.org/jira/browse/MESOS-6035 > > > Repository: mesos > > > Description > ------- > > Added non-recursive version of `cgroups::get`. > > > Diffs > ----- > > src/linux/cgroups.hpp 047d3ea596660e704447c0f0c7b914a43c4a2187 > src/linux/cgroups.cpp f3232c009d04bb82701c0407b12abf999ab60a73 > > Diff: https://reviews.apache.org/r/51031/diff/ > > > Testing > ------- > > > Thanks, > > haosdent huang > >
