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