-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38580/#review105579
-----------------------------------------------------------



src/slave/containerizer/mesos/provisioner/docker/remote_puller.hpp (line 45)
<https://reviews.apache.org/r/38580/#comment164189>

    We pull multiple images with each puller, I would clarify that "Pulls 
docker images from docker registry"



src/slave/containerizer/mesos/provisioner/docker/remote_puller.hpp (line 47)
<https://reviews.apache.org/r/38580/#comment164190>

    We had quite some feeedback about not really knowing what does Remote mean 
in RemotePuller. How about let's rename this to RegistryPuller instead?
    
    I think this matches what it does more closely, since a Registry could 
technically even live in the same box.



src/slave/containerizer/mesos/provisioner/docker/remote_puller.cpp (line 51)
<https://reviews.apache.org/r/38580/#comment164191>

    This is no longer true since we pulled these out of RemotePuller right? 
Same as all the other ones.



src/slave/containerizer/mesos/provisioner/docker/remote_puller.cpp (line 131)
<https://reviews.apache.org/r/38580/#comment164198>

    Move this into RegistryPullerProcess, introduce a new method inside to call 
after with.



src/slave/containerizer/mesos/provisioner/docker/remote_puller.cpp (line 246)
<https://reviews.apache.org/r/38580/#comment164192>

    Can there be layers that actually have zero content? Docker images AFAIK 
has the last layer left empty as a mutable layer, so I'm wondering if you could 
actually get empty layer from an image.



src/slave/containerizer/mesos/provisioner/docker/remote_puller.cpp (line 365)
<https://reviews.apache.org/r/38580/#comment164193>

    I would delete onDiscard and onFailed here.
    Not sure if moving this to the top makes any difference.



src/slave/containerizer/mesos/provisioner/docker/remote_puller.cpp (line 376)
<https://reviews.apache.org/r/38580/#comment164194>

    you don't need process::defer here? You skipped the namespace here but used 
it in Line 226



src/slave/containerizer/mesos/provisioner/docker/remote_puller.cpp (line 381)
<https://reviews.apache.org/r/38580/#comment164195>

    Fix the indentation as we discussed earlier,
    
    foreach (const .......,
             manifest.fsLayerInfos) {
             
    }



src/slave/containerizer/mesos/provisioner/docker/remote_puller.cpp (line 391)
<https://reviews.apache.org/r/38580/#comment164196>

    Move these before the return line.



src/slave/containerizer/mesos/provisioner/docker/remote_puller.cpp (line 394)
<https://reviews.apache.org/r/38580/#comment164197>

    Can we move these callbacks into smaller named methods? 
    
    I think it makes the flow much clearer:
    
    return registryClient->getManifest()
        .then(.... return downloadLayers()
        .then(.... return untarLayers())


- Timothy Chen


On Nov. 7, 2015, 12:11 a.m., Jojy Varghese wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38580/
> -----------------------------------------------------------
> 
> (Updated Nov. 7, 2015, 12:11 a.m.)
> 
> 
> Review request for mesos, Jie Yu, Timothy Chen, and Jiang Yan Xu.
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Integrated remote puller with store. Tests will follow.
> 
> 
> Diffs
> -----
> 
>   src/CMakeLists.txt cbc25e3ea33c47a787d34a7fa8499af3eb0b2c10 
>   src/Makefile.am 736f81252712bc4eee2c114535c325777a85a78f 
>   src/slave/containerizer/mesos/provisioner/docker/local_puller.hpp 
> 87d80026939a326bd1169f46906e36d6ef19f546 
>   src/slave/containerizer/mesos/provisioner/docker/local_puller.cpp 
> f314f20d989ed0125e15d2a14d0f8e56c56976d5 
>   src/slave/containerizer/mesos/provisioner/docker/puller.hpp 
> 8010b8aa75a2fc2e4b537f915f9dca028e407b63 
>   src/slave/containerizer/mesos/provisioner/docker/puller.cpp 
> f61f9e5e9e97a771a03b75ff17cfcd3e3ecbc9b2 
>   src/slave/containerizer/mesos/provisioner/docker/registry_client.hpp 
> bc487d34e4087fa53452d63416d0f31e77060eb0 
>   src/slave/containerizer/mesos/provisioner/docker/remote_puller.hpp 
> PRE-CREATION 
>   src/slave/containerizer/mesos/provisioner/docker/remote_puller.cpp 
> PRE-CREATION 
>   src/slave/containerizer/mesos/provisioner/docker/store.cpp 
> bb02d650e16d45fcf337a7954f7a26143fb2c69f 
>   src/slave/flags.hpp 3e93b52a5874f52361d5a9c685499a7032014a73 
>   src/slave/flags.cpp ed9b0b8313f5a5e53f3715af5300d9fcaa936df8 
>   src/tests/containerizer/provisioner_docker_tests.cpp 
> fe6a90fe32364eec8ef923a000db19183603c338 
> 
> Diff: https://reviews.apache.org/r/38580/diff/
> 
> 
> Testing
> -------
> 
> make check.
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>

Reply via email to