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



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

    Replace + with <<



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

    Fix identation, and let's just all use <<



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

    Does this warrent a LOG(INFO)? Perhaps VLOG(1)?
    I suspect this is a pretty normal situation.



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

    Don't need to assign this right? Just return  
_downloadTracker.at(layer.layerId).future()



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

    using namespace process and you can get rid of process:: here, we do that 
everywhere



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

    I thought you wanted to move this to somewhere shared? We can create a base 
puller class and move this there.



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

    You need to return here.



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

    This doesn't look right, it's simply a invalid timeout value that's passed 
to puller, nothing to do with remote name to canonial name



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

    Why break down layer timeout? Why not just one timeout?
    
    And how is the user able to configure this pull timeout in the first place?
    
    If we don't have give users the option to change this I suggest removing 
this from the interface and let's just use constants for now.


- Timothy Chen


On Oct. 1, 2015, 6:40 p.m., Jojy Varghese wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38580/
> -----------------------------------------------------------
> 
> (Updated Oct. 1, 2015, 6:40 p.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/Makefile.am 8aa456611dd5405336dd7b0c19ba4a942ea1c805 
>   src/slave/containerizer/provisioner/docker/local_puller.hpp 
> 4574e8a04663482625d7b54f765741f221ec13e0 
>   src/slave/containerizer/provisioner/docker/local_puller.cpp 
> 4a0b7d11f013941084571f2d89d835a4668a3d8b 
>   src/slave/containerizer/provisioner/docker/puller.hpp 
> 105b4e75439c2ad4c08e2fd364f288f1d39b9b59 
>   src/slave/containerizer/provisioner/docker/puller.cpp 
> cb05324689ffa26ce830b513e2d71b55517da3cb 
>   src/slave/containerizer/provisioner/docker/remote_puller.hpp PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/remote_puller.cpp PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/store.cpp 
> cbb67686d45513f0395a0cf1bc5c43cb4935adae 
>   src/slave/flags.hpp f76f0f6a0c7a01c7f061a225d7f6ef52be0ee7b5 
>   src/slave/flags.cpp 029aa1eb00e1aa3e92c2155925022c17fd905862 
> 
> Diff: https://reviews.apache.org/r/38580/diff/
> 
> 
> Testing
> -------
> 
> make check.
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>

Reply via email to