This is an automatically generated e-mail. To reply, visit:

First pass. Haven't looked at LocalStore and MetadataManager in details yet.

src/messages/docker_provisioner.proto (line 28)

    I would put this message under 
    since this message is docker provisioner specific.
    ALso, why not unify this with DockerImage and ImageName? I.e.,
    message DockerImage {
      message Name {
      required Name name = 1;
      repeated string layer_ids = 2;
    message DockerImages {
      repeated DockerImage images = 1;

src/slave/containerizer/provisioners/docker.hpp (line 19)

    I'll change the appc one as well.
    Also, I think this should be put into
    I'll change the appc one accordingly as well.

src/slave/containerizer/provisioners/docker.hpp (line 54)

    Two lines apart.

src/slave/containerizer/provisioners/docker.hpp (line 59)

    Can you put ImageName and DockerImage into a separate file under 
    I feels strange when provisioner header depends on store header and store 
header depends on provisioner header.
    Also, a second thought.
    Why not make 'ImageName' and 'DockerImage' a protobuf as well?

src/slave/containerizer/provisioners/docker.hpp (line 61)


src/slave/containerizer/provisioners/docker.hpp (line 88)

    Any reason those fields are not const?

src/slave/containerizer/provisioners/docker.hpp (line 112)

    Can you just call it Image given it's under docker namespace.

src/slave/containerizer/provisioners/docker.hpp (line 124)

    Two lines apart.

src/slave/containerizer/provisioners/docker.hpp (line 127)

    Two lines apart.

src/slave/containerizer/provisioners/docker.hpp (line 159)

    Kill one blank line here.

src/slave/containerizer/provisioners/docker.cpp (lines 59 - 61)

    Why do you need this function. Can you move all the logics in this function 
to DockerProvisioner::create?
    Feels free to make DockerProvisionerProcess constructor public because it's 
in a source file (i.e., hidden).

src/slave/containerizer/provisioners/docker.cpp (line 100)

    Please move this function to a separate source file.

src/slave/containerizer/provisioners/docker.cpp (line 120)

    Two lines apart. Please do a sweep to fix all such occurances.

src/slave/containerizer/provisioners/docker.cpp (lines 243 - 248)

    OK, this is problematic if default container info is used. What if the 
framework launches a task that does not have container info specified, but 
mesos slave provides a default container info. The container will be treated as 
unknown orphans and thus destroyed.
    FYI, we checkpoint the ExecutorInfo/TaskInfo before filling the default 
container info.
    What you need to do here is to remove the 'if' check here.

src/slave/containerizer/provisioners/docker.cpp (line 306)

    "Destroying rootfs '" << rootfs << "' from an unknown orphan container " << 

src/slave/containerizer/provisioners/docker/local_store.hpp (lines 48 - 50)

    Please reorder these two methods.

src/slave/containerizer/provisioners/docker/local_store.cpp (line 19)

    Please fix the include order please.

src/slave/containerizer/provisioners/docker/local_store.cpp (lines 60 - 62)

    Similar to provisioner, you can get rid of this 'create' function, instead 
you can expose LocalStoreProcess constructor.

src/slave/containerizer/provisioners/docker/local_store.cpp (lines 103 - 116)

    Do you want to put this in store.cpp?

src/slave/containerizer/provisioners/docker/local_store.cpp (lines 191 - 193)

    Put the lambda into `_get` seems to be more readable to me. Binding 'this' 
is not recommended in general because this will become dangling when this 
object is deleted (althought it's not possible in this case because of the 
'defer', but it's not quite clear).

src/slave/containerizer/provisioners/docker/local_store.cpp (line 223)

    2 lines please.

src/slave/flags.cpp (line 77)

    `docker_store` sounds too generic. How about `docker_store_discovery`

src/slave/flags.cpp (line 88)

    'docker_backend' sounds too generic. Please rename this to:
    We'll do the appc renaming as well.

src/slave/flags.cpp (line 92)


- Jie Yu

On Sept. 9, 2015, 5:03 p.m., Timothy Chen wrote:
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38137/
> -----------------------------------------------------------
> (Updated Sept. 9, 2015, 5:03 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 0a8ef6d 
>   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/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/metadata_manager.hpp 
>   src/slave/containerizer/provisioners/docker/metadata_manager.cpp 
>   src/slave/containerizer/provisioners/docker/paths.hpp PRE-CREATION 
>   src/slave/containerizer/provisioners/docker/paths.cpp PRE-CREATION 
>   src/slave/containerizer/provisioners/docker/store.hpp 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

Reply via email to