----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37198/#review96523 -----------------------------------------------------------
src/Makefile.am (lines 747 - 748) <https://reviews.apache.org/r/37198/#comment151949> Formatting off, use hard-tabs for these files please. src/slave/containerizer/provisioner.hpp (line 23) <https://reviews.apache.org/r/37198/#comment151952> Missing ``` #include <string> ``` src/slave/containerizer/provisioner.hpp (lines 60 - 61) <https://reviews.apache.org/r/37198/#comment151951> Should we adapt the comment to mention option to specify the sandbox location? src/slave/containerizer/provisioner.cpp (line 23) <https://reviews.apache.org/r/37198/#comment151950> See my previous comment on "using namespace ...". src/slave/containerizer/provisioner.cpp (line 46) <https://reviews.apache.org/r/37198/#comment151953> One space less for the indentation. src/slave/containerizer/provisioners/docker.hpp (line 29) <https://reviews.apache.org/r/37198/#comment151956> Missing: ``` #include <stout/nothing.hpp> ``` src/slave/containerizer/provisioners/docker.hpp (line 31) <https://reviews.apache.org/r/37198/#comment151954> Missing: ``` #include <stout/option.hpp> ``` src/slave/containerizer/provisioners/docker.hpp (line 107) <https://reviews.apache.org/r/37198/#comment151955> You are declaring virtual functions but your destructor is not virtual => aren't we reaching UB-land once this class gets dynamically created via its factory and later on destroyed via the smartpointer? src/slave/containerizer/provisioners/docker.hpp (line 121) <https://reviews.apache.org/r/37198/#comment151958> explicit? src/slave/containerizer/provisioners/docker.cpp (line 18) <https://reviews.apache.org/r/37198/#comment152032> Missing ``` #include <glog/logging.hpp> ``` src/slave/containerizer/provisioners/docker.cpp (line 31) <https://reviews.apache.org/r/37198/#comment151967> This should be the top include. src/slave/containerizer/provisioners/docker.cpp (line 49) <https://reviews.apache.org/r/37198/#comment151957> Remove this line please. src/slave/containerizer/provisioners/docker.cpp (line 206) <https://reviews.apache.org/r/37198/#comment151960> You generally seem to prefer "initializing tightly followed by validation" but here you insert a blank line -- let's make this consistent. src/slave/containerizer/provisioners/docker.cpp (lines 242 - 243) <https://reviews.apache.org/r/37198/#comment151962> Seems to be well below our 80 chars limit - could do without the wrapping. src/slave/containerizer/provisioners/docker/backend.hpp (line 29) <https://reviews.apache.org/r/37198/#comment151963> Missing ``` #include <process/owned.hpp> ``` src/slave/containerizer/provisioners/docker/backend.hpp (line 39) <https://reviews.apache.org/r/37198/#comment151990> You should unify the "Docker" case -- make sure you always call it "Docker" or "docker" in your comments. src/slave/containerizer/provisioners/docker/backend.hpp (line 58) <https://reviews.apache.org/r/37198/#comment151965> Kill this blank line please. We generally add a blank line after "closing a scope" and one before "openening a scope" -- it may be a bit vague as I call it out here, but that is how I remind myself :). src/slave/containerizer/provisioners/docker/backend.cpp (line 21) <https://reviews.apache.org/r/37198/#comment152033> Missing ``` #include <glog/logging.h> ``` src/slave/containerizer/provisioners/docker/backend.cpp (line 27) <https://reviews.apache.org/r/37198/#comment151969> Top include! src/slave/containerizer/provisioners/docker/backend.cpp (line 155) <https://reviews.apache.org/r/37198/#comment151975> See my earlier comments on the status-code/exit-code mixup. src/slave/containerizer/provisioners/docker/backend.cpp (lines 166 - 174) <https://reviews.apache.org/r/37198/#comment151979> Why using rm via subprocess instead of os::rmdir? - Till Toenshoff On Aug. 25, 2015, 8:58 p.m., Lily Chen wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/37198/ > ----------------------------------------------------------- > > (Updated Aug. 25, 2015, 8:58 p.m.) > > > Review request for mesos, Ian Downes, Jie Yu, Jojy Varghese, Timothy Chen, > and Jiang Yan Xu. > > > Bugs: MESOS-2850 > https://issues.apache.org/jira/browse/MESOS-2850 > > > Repository: mesos > > > Description > ------- > > Add Docker image provisioner and copy backend. > > > Diffs > ----- > > src/Makefile.am 571e1ac0f96b2452797a478680b540f2aab63aab > src/slave/containerizer/isolators/filesystem/linux.cpp > f36424e94c380870cfde49d55af397fa3dc4a612 > src/slave/containerizer/provisioner.hpp > 541dd4e0b2f0c92a45c00cab6132a2be69654838 > src/slave/containerizer/provisioner.cpp > efc7e6996ff6663bebaf61989a7e040bd2ad7a5e > src/slave/containerizer/provisioners/docker.hpp PRE-CREATION > src/slave/containerizer/provisioners/docker.cpp PRE-CREATION > src/slave/containerizer/provisioners/docker/backend.hpp PRE-CREATION > src/slave/containerizer/provisioners/docker/backend.cpp PRE-CREATION > src/slave/flags.hpp e56738e2dfd6593ef8f093687919da287af78f77 > src/slave/flags.cpp b36710d6d7a7250bc071a57310a2d54bfb3bc624 > src/tests/containerizer/provisioner.hpp > c4ba46794fe5d7875fda11155367f521c34ea339 > > Diff: https://reviews.apache.org/r/37198/diff/ > > > Testing > ------- > > make check > > > Thanks, > > Lily Chen > >
