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