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 38580: Added docker registry RemotePuller
Date Thu, 01 Oct 2015 18:53:11 GMT

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


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