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 37921: Add Copy backend for provisioners.
Date Mon, 31 Aug 2015 17:50:24 GMT

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



src/Makefile.am (line 629)
<https://reviews.apache.org/r/37921/#comment152842>

    copy.cpp is not Linux only.



src/slave/containerizer/provisioners/backends/copy.cpp (line 19)
<https://reviews.apache.org/r/37921/#comment152710>

    Move this below.
    
    ```
    #include "common/status_utils.hpp"
    
    #include "slave/containerizer/provisioners/backends/copy.hpp"
    ```



src/slave/containerizer/provisioners/backends/copy.cpp (line 25)
<https://reviews.apache.org/r/37921/#comment152711>

    defer.hpp goes before dispatch.hpp



src/slave/containerizer/provisioners/backends/copy.cpp (line 109)
<https://reviews.apache.org/r/37921/#comment152712>

    Move '{' to the end of the last line.



src/slave/containerizer/provisioners/backends/copy.cpp (line 117)
<https://reviews.apache.org/r/37921/#comment152780>

    Failure in Backend::provision() will ultimately invoke Backend::destroy() by the Slave
so this is not necessary.



src/slave/containerizer/provisioners/backends/copy.cpp (lines 127 - 132)
<https://reviews.apache.org/r/37921/#comment152783>

    Using shell expansion here has others issues:
    
    1) Spaces (or other $IFS) in the paths
    2) Not sure if filename expansions works the same way across shells.
    
    GNU `cp` has a `--no-target-directory` flag that's handy in this situation but Mac `cp`
doesn't have it.
    
    So I think we should probably just rely on a precondition check: return Failure if os::exists(rootfs)
already exists and remove the mkdir in `provision()`. This allows us to just use the *argv*
form of `subprocess` with `{"cp", "-a", layer, rootfs}`.
    
    AppcProvisioner already expects the Backend to create the `rootfs` and DockerProvisioner
can do the same. 
    
    We can emphaize this by adding a note to `Backend::provision()` documentation.
    
    Thoughts?



src/slave/containerizer/provisioners/backends/copy.cpp (lines 139 - 148)
<https://reviews.apache.org/r/37921/#comment152781>

    Two space indentation for the body of the lambda and the closing `}` aligned with the
`.`.



src/slave/containerizer/provisioners/backends/copy.cpp (lines 168 - 177)
<https://reviews.apache.org/r/37921/#comment152782>

    Ditto about indentation.



src/tests/containerizer/provisioner_backend_tests.cpp (line 142)
<https://reviews.apache.org/r/37921/#comment152784>

    Blank line before.



src/tests/containerizer/provisioner_backend_tests.cpp (lines 149 - 150)
<https://reviews.apache.org/r/37921/#comment152840>

    One blank line here.


- Jiang Yan Xu


On Aug. 29, 2015, 12:59 a.m., Timothy Chen wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37921/
> -----------------------------------------------------------
> 
> (Updated Aug. 29, 2015, 12:59 a.m.)
> 
> 
> Review request for mesos, Jie Yu and Jiang Yan Xu.
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Add Copy backend for provisioners.
> 
> 
> Diffs
> -----
> 
>   src/Makefile.am 7b620ff66856b3f0adac121b3297d55ed71a3d99 
>   src/slave/containerizer/provisioners/backend.cpp 2f7c335f62fdeb27526ab9a38a07c097422ae92b

>   src/slave/containerizer/provisioners/backends/copy.hpp PRE-CREATION 
>   src/slave/containerizer/provisioners/backends/copy.cpp PRE-CREATION 
>   src/tests/containerizer/provisioner_backend_tests.cpp d321850613223a2357ca1646a9d988d05171772c

> 
> Diff: https://reviews.apache.org/r/37921/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Timothy Chen
> 
>


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