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 37773: Docker: Adding registry client.
Date Tue, 01 Sep 2015 21:35:29 GMT

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



src/slave/containerizer/provisioners/docker/registry_client.hpp (line 34)
<https://reviews.apache.org/r/37773/#comment153180>

    Remove provisioners namespace for now



src/slave/containerizer/provisioners/docker/registry_client.hpp (line 34)
<https://reviews.apache.org/r/37773/#comment153181>

    Remove provisioners namespace for now



src/slave/containerizer/provisioners/docker/registry_client.cpp (line 50)
<https://reviews.apache.org/r/37773/#comment153186>

    spell out credentials



src/slave/containerizer/provisioners/docker/registry_client.cpp (line 67)
<https://reviews.apache.org/r/37773/#comment153187>

    ditto



src/slave/containerizer/provisioners/docker/registry_client.cpp (line 93)
<https://reviews.apache.org/r/37773/#comment153188>

    fix ident (4 spaces, so move 2 spaces left)



src/slave/containerizer/provisioners/docker/registry_client.cpp (line 108)
<https://reviews.apache.org/r/37773/#comment153189>

    We should move all the logic into process, see similiar comment made in AppProvisionerProcess
(https://reviews.apache.org/r/37881/diff/2?file=1059946#file1059946line174)



src/slave/containerizer/provisioners/docker/registry_client.cpp (line 186)
<https://reviews.apache.org/r/37773/#comment153183>

    What's the benefits for this inline lambda vs just putting this code in the foreach loop?
    
    I don't see you reuse it mulitple places and there is only one single call site.



src/slave/containerizer/provisioners/docker/registry_client.cpp (line 214)
<https://reviews.apache.org/r/37773/#comment153201>

    Can you add some verbose logging especially when we're calling ourselves again?
    
    This seems like code that we can get into trouble especially when the docker registry
implementation changes (ie: they start returning 202 instead of 200, or infinite recirusion
starts happening, etc).



src/slave/containerizer/provisioners/docker/registry_client.cpp (line 219)
<https://reviews.apache.org/r/37773/#comment153190>

    This should be Option<string> right? Since we might not have a lastResponseStatus.
And we can default it to None() so all the callers for this method can just skip that instead
of passing in a ""
    
    Can you also comment on why we need this?



src/slave/containerizer/provisioners/docker/registry_client.cpp (line 233)
<https://reviews.apache.org/r/37773/#comment153191>

    You should state the assumption in a resend that we don't expect a response status that
causes another resend, which can still cause infinitte recursion.



src/slave/containerizer/provisioners/docker/registry_client.cpp (line 235)
<https://reviews.apache.org/r/37773/#comment153200>

    You did "Invalid Response :" + error in the bottom, and "Invalid Response '" + error +
"'" here.
    
    Let's just keep the same format, I suggest the former.
    
    Also I think worth mentioning that this is matching the last response.



src/slave/containerizer/provisioners/docker/registry_client.cpp (line 266)
<https://reviews.apache.org/r/37773/#comment153192>

    The identation seems off, I think you should probably just move the self() to the last
line and the beginning of lambda too.



src/slave/containerizer/provisioners/docker/registry_client.cpp (line 281)
<https://reviews.apache.org/r/37773/#comment153193>

    Can you also add a comment that we need to add redirect functionality into libprocess
too? Once we have that we don't have to handle it ourselves



src/slave/containerizer/provisioners/docker/registry_client.cpp (line 291)
<https://reviews.apache.org/r/37773/#comment153195>

    strings::contains



src/slave/containerizer/provisioners/docker/registry_client.cpp (line 293)
<https://reviews.apache.org/r/37773/#comment153194>

    Fix alignment



src/slave/containerizer/provisioners/docker/registry_client.cpp (line 306)
<https://reviews.apache.org/r/37773/#comment153196>

    Put this in a constant.



src/slave/containerizer/provisioners/docker/registry_client.cpp (line 309)
<https://reviews.apache.org/r/37773/#comment153197>

    Can you comment what's this block is for?



src/slave/containerizer/provisioners/docker/registry_client.cpp (line 315)
<https://reviews.apache.org/r/37773/#comment153198>

    Fix indentation.



src/slave/containerizer/provisioners/docker/registry_client.cpp (line 336)
<https://reviews.apache.org/r/37773/#comment153199>

    Fix alignment



src/slave/containerizer/provisioners/docker/registry_client.cpp (line 359)
<https://reviews.apache.org/r/37773/#comment153204>

    Should we make sure somewhere that we encode or check the tag so that we don't contain
spaces?



src/slave/containerizer/provisioners/docker/registry_client.cpp (line 382)
<https://reviews.apache.org/r/37773/#comment153202>

    Move this to previous line



src/slave/containerizer/provisioners/docker/registry_client.cpp (line 401)
<https://reviews.apache.org/r/37773/#comment153203>

    don't take reference for size_t, just pass by value



src/slave/containerizer/provisioners/docker/registry_client.cpp (line 410)
<https://reviews.apache.org/r/37773/#comment153208>

    I think ideally we can introduce something in libprocess so we can stream results to disk
with splice or something like that, avoid as much copies as we can.



src/tests/provisioners/docker_provisioner_tests.cpp (line 34)
<https://reviews.apache.org/r/37773/#comment153209>

    This goes before token_manager



src/tests/provisioners/docker_provisioner_tests.cpp (line 231)
<https://reviews.apache.org/r/37773/#comment153213>

    extra space between if and (



src/tests/provisioners/docker_provisioner_tests.cpp (line 232)
<https://reviews.apache.org/r/37773/#comment153214>

    Why is this needed?


- Timothy Chen


On Sept. 1, 2015, 12:02 a.m., Jojy Varghese wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37773/
> -----------------------------------------------------------
> 
> (Updated Sept. 1, 2015, 12:02 a.m.)
> 
> 
> Review request for mesos, Lily Chen and Timothy Chen.
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Added implementation for docker registry's Get Manifest and Get Blob APIs.
> 
> 
> Diffs
> -----
> 
>   src/Makefile.am 7b4d9f65506e7fa8425966009401aae73cdb79a5 
>   src/slave/containerizer/provisioners/docker/registry_client.hpp PRE-CREATION 
>   src/slave/containerizer/provisioners/docker/registry_client.cpp PRE-CREATION 
>   src/tests/provisioners/docker_provisioner_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/37773/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>


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