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

Thanks for taking this on Jojy! This is going to be a great exercise in sending 
out small patches so we land things quickly :)

First thing is that this review depends on the addition of new functionality in 
https://reviews.apache.org/r/38443/ . It's easier for the reviewers to help you 
land your cleanups without it being blocked on the new functionality, can you 
remove the dependency?

I wasn't able to do a complete pass on everything since there is a lot of code 
here. But to get things started, I've done an initial pass and made a number of 
comments below. Let's start by addressing them and using small independent 
patches for each cleanup, that way we can make progress without having to clean 
everything up all in one patch.

Much appreciated!!

src/slave/containerizer/provisioner/docker/registry_client.hpp (lines 36 - 40)

    Hm.. any reason the registry client is nested within the containerizer?
    A registry client seems like a general abstraction, should it live up 
alongside the `Docker` abstraction within src/docker?

src/slave/containerizer/provisioner/docker/registry_client.hpp (line 46)

    Can we avoid the redundant naming here? We currently have 
`docker::registry::RegistryClient`, how about `docker::registry::Client` or 

src/slave/containerizer/provisioner/docker/registry_client.hpp (lines 53 - 54)

    Hm.. can you fix the style on your comments? There should be a space after 
the `//`.

src/slave/containerizer/provisioner/docker/registry_client.hpp (line 64)

    Why ManifestResponse instead of just Manifest..?

src/slave/containerizer/provisioner/docker/registry_client.hpp (line 67)

    How about: s/fsLayerInfoList/layers/

src/slave/containerizer/provisioner/docker/registry_client.hpp (line 71)

    Authentication or authorization? Can you avoid the abbreviation? This is 
what we've done for the rest of our authentication/authorization code.

src/slave/containerizer/provisioner/docker/registry_client.hpp (lines 75 - 86)

    As it stands these comments don't seem to be adding any value over what the 
code expresses, should we remove them? Or is there anything that you think they 
should call out that the reader can't see from the variable names?

src/slave/containerizer/provisioner/docker/registry_client.hpp (line 100)

    authentication or authorization, or both? Can you do a pass for this 

src/slave/containerizer/provisioner/docker/registry_client.hpp (line 147)

    We've been trying to eliminate static non-PODs, could you replace this with 
a static function?

src/slave/containerizer/provisioner/docker/registry_client.cpp (lines 62 - 65)

    Hm.. why did you need this additional static create? Looking at the 
implementation it seems as though you only need `RegistryClient::create`.

src/slave/containerizer/provisioner/docker/registry_client.cpp (line 106)

    How about s/getManifestResponse/getManifest/

src/slave/containerizer/provisioner/docker/registry_client.cpp (lines 122 - 130)

    Generally our pattern for process wrappers is to have the wrapper 
constructor allocate the process. Here it is getting injected. Can you refactor 
this to make it consistent? Ditto for TokenManager.
    This means avoiding RegistryClientProcess::create and creating the token 
manager in here, then passing everything we need into the RegistryClient 
constructor, which is a simple pass through to the RegistryClientProcess 
    Looking at the TokenManager code, I'm confused why TokenManager::create 
exists, and why there is a Try. It looks like token manager construction never 
returns an error?
    If we clean up the token manager creation code in the same way as I'm 
suggesting here, we can make this a lot simpler as well. :)

src/slave/containerizer/provisioner/docker/registry_client.cpp (line 193)

    We try to avoid abbreviations for names like this, can you do a pass to 
clean them up?

src/slave/containerizer/provisioner/docker/registry_client.cpp (line 231)

    Please do a pass for avoiding abbreviations like "auth" and "param", why 
not just s/authParams/tokens/ here?

src/slave/containerizer/provisioner/docker/registry_client.cpp (lines 234 - 248)

    This lambda is pulling in a local variable and mutating it, which makes 
things pretty implicit later when you use it:
      foreach (const string& param, authParams) {
        Try<Nothing> addRes = addAttribute(param); // This is mutating local 
        if (addRes.isError()) {
          return Error(addRes.error());
    How about we keep the mutation explicit here by avoiding the lambda?

src/slave/containerizer/provisioner/docker/registry_client.cpp (lines 481 - 482)

    Why is this a member function? Can it be a standalone function for 
converting a response into a manifest?

src/slave/containerizer/provisioner/docker/registry_client.cpp (line 526)

    Strange that size_t is used here which I'm guessing is why you stuck the 
post-decrement in the loop condition. That's a bit tricky, can you just use an 
rbegin iterator here?

src/slave/containerizer/provisioner/docker/registry_client.cpp (line 600)

    Let's just use = here for readability:
    URL url = registryServer_;
    Ditto below in getBlob and please do a pass for others.

src/slave/containerizer/provisioner/docker/registry_client.cpp (lines 626 - 638)

    Let's avoid this lambda entirely please, it appears to just be adding more 
code than necessary.

src/slave/containerizer/provisioner/docker/registry_client.cpp (line 627)

    Why introduce a variable for this when it's only used once in os::mkdir?

src/slave/containerizer/provisioner/docker/registry_client.cpp (line 630)

    The pattern we use is generally:
    Try<Nothing> mkdir = os::mkdir(...);
    Can you please do a sweep?

src/slave/containerizer/provisioner/docker/registry_client.cpp (lines 640 - 641)

    What is the residue?

src/slave/containerizer/provisioner/docker/registry_client.cpp (lines 646 - 648)

    Why only this validation? Should 'path' be a 'Path'?

src/slave/containerizer/provisioner/docker/registry_client.cpp (lines 654 - 671)

    Is it acceptable to hold the whole blob in memory like this?? How big can 
these blobs be?

- Ben Mahler

On Oct. 2, 2015, 12:24 a.m., Jojy Varghese wrote:
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38579/
> -----------------------------------------------------------
> (Updated Oct. 2, 2015, 12:24 a.m.)
> Review request for mesos, Ben Mahler and Timothy Chen.
> Repository: mesos
> Description
> -------
> - Moved ManifestResponse struct from RegistryClient to namespace.
> - Cleanup
> Diffs
> -----
>   src/slave/containerizer/provisioner/docker/registry_client.hpp 
> 9d5d15455192e2d75fe5cd7fa8755fb8cc67e185 
>   src/slave/containerizer/provisioner/docker/registry_client.cpp 
> c2040b48ea43fdb29766994c244273d3fa9ee3cd 
>   src/tests/containerizer/provisioner_docker_tests.cpp 
> d895eb9d0723e52cff8b21ef2deeaef1911d019c 
> Diff: https://reviews.apache.org/r/38579/diff/
> Testing
> -------
> make check.
> Thanks,
> Jojy Varghese

Reply via email to