mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Ben Mahler" <benjamin.mah...@gmail.com>
Subject Re: Review Request 38579: Refactored registry client
Date Fri, 02 Oct 2015 23:16:13 GMT

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


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)
<https://reviews.apache.org/r/38579/#comment158754>

    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)
<https://reviews.apache.org/r/38579/#comment158752>

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



src/slave/containerizer/provisioner/docker/registry_client.hpp (lines 53 - 54)
<https://reviews.apache.org/r/38579/#comment158757>

    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)
<https://reviews.apache.org/r/38579/#comment158783>

    Why ManifestResponse instead of just Manifest..?



src/slave/containerizer/provisioner/docker/registry_client.hpp (line 67)
<https://reviews.apache.org/r/38579/#comment158756>

    How about: s/fsLayerInfoList/layers/



src/slave/containerizer/provisioner/docker/registry_client.hpp (line 71)
<https://reviews.apache.org/r/38579/#comment158755>

    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)
<https://reviews.apache.org/r/38579/#comment158758>

    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)
<https://reviews.apache.org/r/38579/#comment158759>

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



src/slave/containerizer/provisioner/docker/registry_client.hpp (line 147)
<https://reviews.apache.org/r/38579/#comment158760>

    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)
<https://reviews.apache.org/r/38579/#comment158764>

    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)
<https://reviews.apache.org/r/38579/#comment158772>

    How about s/getManifestResponse/getManifest/



src/slave/containerizer/provisioner/docker/registry_client.cpp (lines 122 - 130)
<https://reviews.apache.org/r/38579/#comment158770>

    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 constructor.
    
    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)
<https://reviews.apache.org/r/38579/#comment158771>

    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)
<https://reviews.apache.org/r/38579/#comment158784>

    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)
<https://reviews.apache.org/r/38579/#comment158773>

    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 state!
        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)
<https://reviews.apache.org/r/38579/#comment158782>

    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)
<https://reviews.apache.org/r/38579/#comment158774>

    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)
<https://reviews.apache.org/r/38579/#comment158781>

    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)
<https://reviews.apache.org/r/38579/#comment158776>

    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)
<https://reviews.apache.org/r/38579/#comment158778>

    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)
<https://reviews.apache.org/r/38579/#comment158777>

    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)
<https://reviews.apache.org/r/38579/#comment158775>

    What is the residue?



src/slave/containerizer/provisioner/docker/registry_client.cpp (lines 646 - 648)
<https://reviews.apache.org/r/38579/#comment158779>

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



src/slave/containerizer/provisioner/docker/registry_client.cpp (lines 654 - 671)
<https://reviews.apache.org/r/38579/#comment158780>

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


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