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 38141: Refactor shared paths in provisioners.
Date Tue, 08 Sep 2015 22:56:14 GMT

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

Ship it!


LGTM!

Could you run `sudo make check GTEST_FILTER="*Provisioner*"` to make sure Appc tests pass?

I originally consolidated all path manipulations into a single paths.hpp|cpp but now with
provisioner dir part pulled out it may make sense to put the store specific ones into the
store itself, but that's for another patch I will take on myself.


src/slave/containerizer/provisioners/appc/paths.cpp (lines 21 - 23)
<https://reviews.apache.org/r/38141/#comment154347>

    No longer needed.



src/slave/containerizer/provisioners/appc/paths.cpp (line 25)
<https://reviews.apache.org/r/38141/#comment154349>

    Ditto.



src/slave/containerizer/provisioners/appc/paths.cpp (line 28)
<https://reviews.apache.org/r/38141/#comment154350>

    Ditto.



src/slave/containerizer/provisioners/appc/paths.cpp (line 31)
<https://reviews.apache.org/r/38141/#comment154352>

    Ditto.



src/slave/containerizer/provisioners/appc/paths.cpp (line 87)
<https://reviews.apache.org/r/38141/#comment154353>

    A single blank line.



src/slave/containerizer/provisioners/paths.hpp (lines 46 - 48)
<https://reviews.apache.org/r/38141/#comment154343>

    This comment was assuming this was for APPC. Now that it's generic, it should be something
like:
    
    ```
    // NOTE: Each container could have multiple image types, therefore there
    // can be the same <container_id> directories under each provisioner e.g.,
    // <work_dir>/provisioners/DOCKER, <work_dir>/provisioners/APPC, etc. 
    ```



src/slave/containerizer/provisioners/paths.hpp (lines 49 - 51)
<https://reviews.apache.org/r/38141/#comment154346>

    There is a "For appc" here, can the same be true for docker? I think it could.


- Jiang Yan Xu


On Sept. 7, 2015, 2:31 p.m., Timothy Chen wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38141/
> -----------------------------------------------------------
> 
> (Updated Sept. 7, 2015, 2:31 p.m.)
> 
> 
> Review request for mesos and Jiang Yan Xu.
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Refactor shared paths in provisioners.
> 
> 
> Diffs
> -----
> 
>   src/Makefile.am 5fdca0f574e7e08c4b1aebed0fac39140c19adfe 
>   src/slave/containerizer/provisioners/appc.cpp fc5ee19df51a3543aaf01c2301b976700610ff57

>   src/slave/containerizer/provisioners/appc/paths.hpp fb3a1a7295809d745dd15bee6db1f7e8dd99ab33

>   src/slave/containerizer/provisioners/appc/paths.cpp e6be851e24886d7c886adad4c7ea29ded17bdcbe

>   src/slave/containerizer/provisioners/paths.hpp PRE-CREATION 
>   src/slave/containerizer/provisioners/paths.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/38141/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Timothy Chen
> 
>


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