> On Sept. 8, 2015, 6:35 p.m., Jojy Varghese wrote: > > src/slave/containerizer/provisioner.hpp, line 54 > > <https://reviews.apache.org/r/38137/diff/1/?file=1064335#file1064335line54> > > > > doxygen style?
I'll fix this later with another patch, this isn't related to docker. > On Sept. 8, 2015, 6:35 p.m., Jojy Varghese wrote: > > src/slave/containerizer/provisioner.cpp, line 36 > > <https://reviews.apache.org/r/38137/diff/1/?file=1064336#file1064336line36> > > > > 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. Ditto. > On Sept. 8, 2015, 6:35 p.m., Jojy Varghese wrote: > > src/slave/containerizer/provisioners/docker.hpp, line 67 > > <https://reviews.apache.org/r/38137/diff/1/?file=1064337#file1064337line67> > > > > I believe the default value of an option is None.So need not explicitly > > declare. It's provided so users can skip providing a value all together. > On Sept. 8, 2015, 6:35 p.m., Jojy Varghese wrote: > > src/slave/containerizer/provisioners/docker.hpp, line 94 > > <https://reviews.apache.org/r/38137/diff/1/?file=1064337#file1064337line94> > > > > Is it layers or just layer ids? If its just layer ids, then the struct > > should be DockerImageInfo (and not DockerImage). Not sure what the difference between DockerImageInfo and DockerImage is. But it's just layer ids. > On Sept. 8, 2015, 6:35 p.m., Jojy Varghese wrote: > > src/slave/containerizer/provisioners/docker.cpp, line 56 > > <https://reviews.apache.org/r/38137/diff/1/?file=1064338#file1064338line56> > > > > I strongly believe that we should have strong/explicit ownership > > semantics for pointers (instead of raw pointers). Sure, perhaps you can have a new patch that fixes from the interface to all the implementations. I rather not do this in this patch. > On Sept. 8, 2015, 6:35 p.m., Jojy Varghese wrote: > > src/slave/containerizer/provisioners/docker.cpp, line 111 > > <https://reviews.apache.org/r/38137/diff/1/?file=1064338#file1064338line111> > > > > should we create a guard here so that multiple calls to "create" wont > > call multiple "process" create? We assume each create is a seperate new process. > On Sept. 8, 2015, 6:35 p.m., Jojy Varghese wrote: > > src/slave/containerizer/provisioners/docker.cpp, line 172 > > <https://reviews.apache.org/r/38137/diff/1/?file=1064338#file1064338line172> > > > > should we check/validate for the docker_rootfs_dir? This is removed. > On Sept. 8, 2015, 6:35 p.m., Jojy Varghese wrote: > > src/slave/containerizer/provisioners/docker.cpp, line 279 > > <https://reviews.apache.org/r/38137/diff/1/?file=1064338#file1064338line279> > > > > since destroy can be called from multiple thread contexts, should we > > proetct it? It's libprocess, so no. > On Sept. 8, 2015, 6:35 p.m., Jojy Varghese wrote: > > src/slave/containerizer/provisioners/docker/local_store.cpp, line 94 > > <https://reviews.apache.org/r/38137/diff/1/?file=1064340#file1064340line94> > > > > s/refStore/referenceStore? Renamed to metadataManager. > On Sept. 8, 2015, 6:35 p.m., Jojy Varghese wrote: > > src/slave/containerizer/provisioners/docker/local_store.cpp, line 157 > > <https://reviews.apache.org/r/38137/diff/1/?file=1064340#file1064340line157> > > > > 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. Thanks this is already fixed in latest revision. > On Sept. 8, 2015, 6:35 p.m., Jojy Varghese wrote: > > src/slave/containerizer/provisioners/docker/local_store.cpp, line 174 > > <https://reviews.apache.org/r/38137/diff/1/?file=1064340#file1064340line174> > > > > Can this return an error? This is already fixed in latest revision - Timothy ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/38137/#review98030 ----------------------------------------------------------- On Sept. 8, 2015, 7:52 p.m., Timothy Chen wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/38137/ > ----------------------------------------------------------- > > (Updated Sept. 8, 2015, 7:52 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 > >
