mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Timothy Chen" <tnac...@apache.org>
Subject Re: Review Request 38137: Added Docker provisioner and local store
Date Wed, 09 Sep 2015 05:04:06 GMT


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


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message