mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Jiang Yan Xu <...@jxu.me>
Subject Re: Review Request 51031: Added non-recursive version of `cgroups::get`.
Date Wed, 17 Aug 2016 19:55:47 GMT

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


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message