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

Reply via email to