----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/38137/#review98030 -----------------------------------------------------------
src/slave/containerizer/provisioner.hpp (line 54) <https://reviews.apache.org/r/38137/#comment154200> doxygen style? src/slave/containerizer/provisioner.cpp (line 36) <https://reviews.apache.org/r/38137/#comment154181> I think we should guard against multiple "create" calls. Call to "create" multiple times here would repeat the whole logic of creating the creator map and provsioners. src/slave/containerizer/provisioners/docker.hpp (line 56) <https://reviews.apache.org/r/38137/#comment154202> doxygen comments? This applies for other places too. src/slave/containerizer/provisioners/docker.hpp (line 65) <https://reviews.apache.org/r/38137/#comment154186> s/repo/repositorypath src/slave/containerizer/provisioners/docker.hpp (line 67) <https://reviews.apache.org/r/38137/#comment154185> I believe the default value of an option is None.So need not explicitly declare. src/slave/containerizer/provisioners/docker.hpp (line 94) <https://reviews.apache.org/r/38137/#comment154187> Is it layers or just layer ids? If its just layer ids, then the struct should be DockerImageInfo (and not DockerImage). src/slave/containerizer/provisioners/docker.hpp (line 121) <https://reviews.apache.org/r/38137/#comment154188> According to google style guide, this should be declared last. Also, why not use the "delete" keyword (c++11)? src/slave/containerizer/provisioners/docker.cpp (line 37) <https://reviews.apache.org/r/38137/#comment154192> maybe explicit namespace names? src/slave/containerizer/provisioners/docker.cpp (line 56) <https://reviews.apache.org/r/38137/#comment154191> I strongly believe that we should have strong/explicit ownership semantics for pointers (instead of raw pointers). src/slave/containerizer/provisioners/docker.cpp (line 89) <https://reviews.apache.org/r/38137/#comment154194> Would be better if we follow the factory patttern (create method that returns a Try)? src/slave/containerizer/provisioners/docker.cpp (line 111) <https://reviews.apache.org/r/38137/#comment154196> should we create a guard here so that multiple calls to "create" wont call multiple "process" create? src/slave/containerizer/provisioners/docker.cpp (line 172) <https://reviews.apache.org/r/38137/#comment154204> should we check/validate for the docker_rootfs_dir? src/slave/containerizer/provisioners/docker.cpp (line 183) <https://reviews.apache.org/r/38137/#comment154205> const? src/slave/containerizer/provisioners/docker.cpp (line 279) <https://reviews.apache.org/r/38137/#comment154207> since destroy can be called from multiple thread contexts, should we proetct it? src/slave/containerizer/provisioners/docker/local_store.hpp (line 34) <https://reviews.apache.org/r/38137/#comment154198> destructor declaration should be after constructor(create)? src/slave/containerizer/provisioners/docker/local_store.hpp (line 53) <https://reviews.apache.org/r/38137/#comment154208> This should be declared last. Applies everywhere. src/slave/containerizer/provisioners/docker/local_store.cpp (line 94) <https://reviews.apache.org/r/38137/#comment154213> s/refStore/referenceStore? src/slave/containerizer/provisioners/docker/local_store.cpp (line 102) <https://reviews.apache.org/r/38137/#comment154211> guard for multiple calls? src/slave/containerizer/provisioners/docker/local_store.cpp (line 157) <https://reviews.apache.org/r/38137/#comment154212> here it is possible that refStore could be not created and we still have a LocalStoreProcess object. Maybe we should move the refStore creation to before line 152. src/slave/containerizer/provisioners/docker/local_store.cpp (line 167) <https://reviews.apache.org/r/38137/#comment154215> const? src/slave/containerizer/provisioners/docker/local_store.cpp (line 174) <https://reviews.apache.org/r/38137/#comment154220> Can this return an error? src/slave/containerizer/provisioners/docker/local_store.cpp (line 179) <https://reviews.apache.org/r/38137/#comment154221> Can this return an error? src/slave/containerizer/provisioners/docker/local_store.cpp (line 280) <https://reviews.apache.org/r/38137/#comment154226> s/layers/layerIds ? src/slave/flags.hpp (line 56) <https://reviews.apache.org/r/38137/#comment154193> Adding more fields to base Flag class might not scale? Should we adopt feature/subsystem specific flags as subclass of base Flags? - Jojy Varghese On Sept. 7, 2015, 11:31 p.m., Timothy Chen wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/38137/ > ----------------------------------------------------------- > > (Updated Sept. 7, 2015, 11:31 p.m.) > > > Review request for mesos, Jojy Varghese, Till Toenshoff, and Jiang Yan Xu. > > > Repository: mesos > > > Description > ------- > > Joining all the commits around provisioner and local store into one review so > it's easier to review, as patches > are changing code on top of each other. > > All the commits are going to committed together. > > > Diffs > ----- > > src/Makefile.am 5fdca0f > src/messages/docker_provisioner.hpp PRE-CREATION > src/messages/docker_provisioner.proto PRE-CREATION > src/slave/containerizer/provisioner.hpp 9e0e0b8 > src/slave/containerizer/provisioner.cpp 95894c0 > src/slave/containerizer/provisioners/appc.cpp fc5ee19 > src/slave/containerizer/provisioners/appc/paths.hpp fb3a1a7 > src/slave/containerizer/provisioners/appc/paths.cpp e6be851 > src/slave/containerizer/provisioners/docker.hpp PRE-CREATION > src/slave/containerizer/provisioners/docker.cpp PRE-CREATION > src/slave/containerizer/provisioners/docker/local_store.hpp PRE-CREATION > src/slave/containerizer/provisioners/docker/local_store.cpp PRE-CREATION > src/slave/containerizer/provisioners/docker/paths.hpp PRE-CREATION > src/slave/containerizer/provisioners/docker/paths.cpp PRE-CREATION > src/slave/containerizer/provisioners/docker/reference_store.hpp > PRE-CREATION > src/slave/containerizer/provisioners/docker/reference_store.cpp > PRE-CREATION > src/slave/containerizer/provisioners/docker/store.hpp PRE-CREATION > src/slave/containerizer/provisioners/paths.hpp PRE-CREATION > src/slave/containerizer/provisioners/paths.cpp PRE-CREATION > src/slave/flags.hpp b8335aa > src/slave/flags.cpp 7539441 > src/tests/containerizer/docker_provisioner_tests.cpp PRE-CREATION > > Diff: https://reviews.apache.org/r/38137/diff/ > > > Testing > ------- > > make check > > > Thanks, > > Timothy Chen > >
