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

src/slave/containerizer/provisioners/docker/registry_client.hpp (line 34)

    Remove provisioners namespace for now

src/slave/containerizer/provisioners/docker/registry_client.hpp (line 34)

    Remove provisioners namespace for now

src/slave/containerizer/provisioners/docker/registry_client.cpp (line 50)

    spell out credentials

src/slave/containerizer/provisioners/docker/registry_client.cpp (line 67)


src/slave/containerizer/provisioners/docker/registry_client.cpp (line 93)

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

src/slave/containerizer/provisioners/docker/registry_client.cpp (line 108)

    We should move all the logic into process, see similiar comment made in 

src/slave/containerizer/provisioners/docker/registry_client.cpp (line 186)

    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 

src/slave/containerizer/provisioners/docker/registry_client.cpp (line 214)

    Can you add some verbose logging especially when we're calling ourselves 
    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)

    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)

    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)

    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)

    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)

    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)


src/slave/containerizer/provisioners/docker/registry_client.cpp (line 293)

    Fix alignment

src/slave/containerizer/provisioners/docker/registry_client.cpp (line 306)

    Put this in a constant.

src/slave/containerizer/provisioners/docker/registry_client.cpp (line 309)

    Can you comment what's this block is for?

src/slave/containerizer/provisioners/docker/registry_client.cpp (line 315)

    Fix indentation.

src/slave/containerizer/provisioners/docker/registry_client.cpp (line 336)

    Fix alignment

src/slave/containerizer/provisioners/docker/registry_client.cpp (line 359)

    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)

    Move this to previous line

src/slave/containerizer/provisioners/docker/registry_client.cpp (line 401)

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

src/slave/containerizer/provisioners/docker/registry_client.cpp (line 410)

    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 

src/tests/provisioners/docker_provisioner_tests.cpp (line 34)

    This goes before token_manager

src/tests/provisioners/docker_provisioner_tests.cpp (line 231)

    extra space between if and (

src/tests/provisioners/docker_provisioner_tests.cpp (line 232)

    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 
>   src/slave/containerizer/provisioners/docker/registry_client.cpp 
>   src/tests/provisioners/docker_provisioner_tests.cpp PRE-CREATION 
> Diff: https://reviews.apache.org/r/37773/diff/
> Testing
> -------
> make check
> Thanks,
> Jojy Varghese

Reply via email to