mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Jie Yu <yujie....@gmail.com>
Subject Re: Review Request 53161: Implemented the conversion from AUFS whiteouts to OverlayFS whiteouts.
Date Thu, 27 Oct 2016 19:49:26 GMT

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/53161/#review154050
-----------------------------------------------------------


Fix it, then Ship it!





src/slave/containerizer/mesos/utils.hpp (line 34)
<https://reviews.apache.org/r/53161/#comment223564>

    I think this method should be moved to `src/slave/containerizer/mesos/provisioner/utils.hpp|cpp`
since this is provision specific.
    
    THe top level utils file is used for general utils used by Mesos containerizer.



src/slave/containerizer/mesos/utils.cpp (line 52)
<https://reviews.apache.org/r/53161/#comment223566>

    I'd rename 'source' to 'directory'.



src/slave/containerizer/mesos/utils.cpp (line 55)
<https://reviews.apache.org/r/53161/#comment223570>

    i'd prefer a new line above



src/slave/containerizer/mesos/utils.cpp (lines 62 - 65)
<https://reviews.apache.org/r/53161/#comment223571>

    Can we do the following to reduce the level of nesting:
    ```
    if (node->fts_info != FTS_F) {
      continue;
    }
    
    if (!strings::startsWith(node->fts_name, docker::spec::WHITEOUT_PREFIX)) {
      continue;
    }
    
    ...
    ```



src/slave/containerizer/mesos/utils.cpp (line 65)
<https://reviews.apache.org/r/53161/#comment223579>

    Do you need `string` here?



src/slave/containerizer/mesos/utils.cpp (lines 68 - 69)
<https://reviews.apache.org/r/53161/#comment223582>

    I'd format like the following:
    ```
    Try<Nothing> setxattr = os::setxattr(
        path.dirname(),
        "trusted.overlay.opaque",
        "y",
        0);
    ```



src/slave/containerizer/mesos/utils.cpp (line 74)
<https://reviews.apache.org/r/53161/#comment223583>

    Why backslash?



src/slave/containerizer/mesos/utils.cpp (lines 92 - 99)
<https://reviews.apache.org/r/53161/#comment223584>

    Can you double check if it is safe to remove files while walking the fts tree? To be more
defensive, i would probably remove them after finishing the walk.


- Jie Yu


On Oct. 25, 2016, 3:46 a.m., Qian Zhang wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53161/
> -----------------------------------------------------------
> 
> (Updated Oct. 25, 2016, 3:46 a.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Bugs: MESOS-6360
>     https://issues.apache.org/jira/browse/MESOS-6360
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Implemented the conversion from AUFS whiteouts to OverlayFS whiteouts.
> 
> 
> Diffs
> -----
> 
>   src/slave/containerizer/mesos/provisioner/docker/store.cpp e192f86a1848b373f3aa73d29688a96375cac313

>   src/slave/containerizer/mesos/utils.hpp 178ebf3effac824e4788d7282795c18dc1cb5265 
>   src/slave/containerizer/mesos/utils.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/53161/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>


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