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

Much closer! Haven't looked at the tests yet but will do.

src/slave/containerizer/provisioner.cpp (line 1)


src/slave/containerizer/provisioner/docker/metadata_manager.hpp (line 57)


src/slave/containerizer/provisioner/docker/metadata_manager.cpp (line 229)


src/slave/containerizer/provisioner/docker/metadata_manager.cpp (line 243)


src/slave/containerizer/provisioner/docker/puller.hpp (line 34)


src/slave/containerizer/provisioner/docker/puller.hpp (line 43)


src/slave/containerizer/provisioner/docker/puller.hpp (line 59)


src/slave/containerizer/provisioner/docker/store.cpp (line 182)


src/slave/flags.hpp (line 54)


src/slave/containerizer/provisioner/docker/local_puller.hpp (line 29)

    Kill line.

src/slave/containerizer/provisioner/docker/local_puller.hpp (line 45)

    Not a strong opinion but if you think this could potentially be "not local" 
(e.g. NFS or HDFS hosting these), you may name it ArchivesPuller because it's 
more about "what the image source format is" then "where it is from".
    We don't, of course, need to implement all the remote cases in the first 

src/slave/containerizer/provisioner/docker/local_puller.cpp (line 83)

    This is not used at all here.

src/slave/containerizer/provisioner/docker/local_puller.cpp (line 119)

    No longer a "local store" if we now use 'store' to refer to the internal 
object and directory structure right? 
    What should we call this? `local archieves`? (`archieves` being a singluar 
word describing the place that these tars are kept).

src/slave/containerizer/provisioner/docker/local_puller.cpp (line 121)

    Add the image name in the error message?
    I am not sure if you need the additional log line above but I guess I don't 
have major objection either.

src/slave/containerizer/provisioner/docker/local_puller.cpp (line 131)

    About the name `staging` here and elsewhere in the file.
    First of all `staging` could be the name of the base staging directory 
under the store root and here it is referring to the temp dir.
    Furthermore, I like the fact that your Puller interface intentionally 
avoids the need to figure out the directory hierarchy but instead takes a 
generic `directory`. 
    Therefore within the puller things should be centered around the 
`directory` and only the caller knows it's a staging temp dir.
    So I suggest s/staging/directory.

src/slave/containerizer/provisioner/docker/local_puller.cpp (line 197)

    What's the problem with it and is it still a problem with recent JSON 
related commits?
    (I have no idea, just would like to check.)

src/slave/containerizer/provisioner/docker/local_puller.cpp (line 237)

    Does it need to be a member method. If not better be written as an private 
free-standing function.

src/slave/containerizer/provisioner/docker/local_puller.cpp (line 288)


src/slave/containerizer/provisioner/docker/local_puller.cpp (line 309)

    I see that this is leaving behind the layer.tar file after untarring, which 
is fine but maybe a comment that it will be cleaned up when the entire staging 
dir is deleted?

src/slave/containerizer/provisioner/docker/metadata_manager.hpp (line 57)

    With the pullers refactored out of the store I don't see the need for a 
separate metadata manager. The metadata is literally "the metadata of the 
store" and it mirrors the set of operations of the store. e.g. Store::recover() 
just calls MetadataManager::recover().
    I am not adamant about making it happen in this review though.

src/slave/containerizer/provisioner/docker/metadata_manager.cpp (lines 210 - 

    Weird to have two colons.
    Change the first `:` to a quote around image name instead?

src/slave/containerizer/provisioner/docker/paths.hpp (line 36)

    This diagram doesn't explain some of the methods in this file involving 
stuff under the "staging" dir below.
    I imagine with a registry puller the temp_dir looks a bit different.
    |-- <temp_dir_archive>
        |-- <layer_id>
            |-- VERSION
            |-- json
            |-- layer.tar

src/slave/containerizer/provisioner/docker/paths.hpp (line 45)


src/slave/containerizer/provisioner/docker/paths.hpp (lines 51 - 63)

    s/LocalImage/ImageArchive/ and
    These methods are decribing what the layout of the "docker image archive" 

src/slave/containerizer/provisioner/docker/puller.hpp (line 34)

    It's unfortunate that the fetcher cannot be used here. I assume it's 
because it lacks certain capabilities but it would be great if we can improve 
the fetcher itself for this task.

src/slave/containerizer/provisioner/docker/puller.hpp (line 44)

    an image

src/slave/containerizer/provisioner/docker/puller.hpp (line 60)

    Kill line.

src/slave/containerizer/provisioner/docker/puller.hpp (line 66)

    Kill line.

src/slave/containerizer/provisioner/docker/puller.cpp (line 41)

    Kill line.

src/slave/containerizer/provisioner/docker/store.hpp (lines 43 - 44)

    This TODO is done, right?

src/slave/containerizer/provisioner/docker/store.cpp (line 166)

    Need to convert layer ID into a layer path right?

src/slave/containerizer/provisioner/docker/store.cpp (line 191)

    How large could this folder potentially be? To remove large directories I 
think a subprocess is more suitable as doing it this way could keep threads 

src/slave/containerizer/provisioner/docker/store.cpp (line 215)

    Two more spaces for indentation.

src/slave/containerizer/provisioner/docker/store.cpp (lines 239 - 240)

    What happens if we already have this layer under "layers"?

src/slave/containerizer/provisioner/docker/store.cpp (lines 247 - 248)

    You don't know it's a `getLocalImageLayerRootfsPath` here (I suggested 
s/LocalImage/ImageArchive) and not "remote image".
    But I saw that you've updated it in a later revision.

src/slave/containerizer/provisioner/docker/store.cpp (line 252)

    Put '+' to the end of the first line.

src/slave/flags.hpp (line 54)

    No longer applicable and I don't think it's in the cpp file?

src/slave/flags.hpp (line 56)

    `docker_local_archives_dir` if we agree on calling the "local place that 
stores the tar images" an **archives**?

src/tests/containerizer/provisioner_docker_tests.cpp (line 699)

    Two blank lines here and elsewhere in this test.

src/slave/containerizer/provisioner/docker/puller.hpp (line 64)

    Kill line.

- Jiang Yan Xu

On Sept. 22, 2015, 3:15 p.m., Timothy Chen wrote:
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38137/
> -----------------------------------------------------------
> (Updated Sept. 22, 2015, 3:15 p.m.)
> Review request for mesos, Jie Yu, 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 e224060 
>   src/slave/containerizer/provisioner/docker/local_puller.hpp PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/local_puller.cpp PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/message.hpp PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/message.proto PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/metadata_manager.hpp 
>   src/slave/containerizer/provisioner/docker/metadata_manager.cpp 
>   src/slave/containerizer/provisioner/docker/paths.hpp PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/paths.cpp PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/puller.hpp PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/puller.cpp PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/store.hpp PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/store.cpp PRE-CREATION 
>   src/slave/containerizer/provisioner/store.cpp 35d1199 
>   src/slave/flags.hpp e31a418 
>   src/slave/flags.cpp add4196 
>   src/tests/containerizer/provisioner_docker_tests.cpp 1b0c304 
> Diff: https://reviews.apache.org/r/38137/diff/
> Testing
> -------
> make check
> Thanks,
> Timothy Chen

Reply via email to