> On Oct. 2, 2015, 11:16 p.m., Ben Mahler wrote: > > src/slave/containerizer/provisioner/docker/registry_client.hpp, lines 75-86 > > <https://reviews.apache.org/r/38579/diff/5/?file=1088841#file1088841line75> > > > > 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?
They only serve doxygen. I thought all new code(header files) has to be commented for generating doxygen based docs. > On Oct. 2, 2015, 11:16 p.m., Ben Mahler wrote: > > src/slave/containerizer/provisioner/docker/registry_client.hpp, line 148 > > <https://reviews.apache.org/r/38579/diff/5/?file=1088841#file1088841line148> > > > > We've been trying to eliminate static non-PODs, could you replace this > > with a static function? I have got rid of this constant in the subsequent patch (https://reviews.apache.org/r/38580) > On Oct. 2, 2015, 11:16 p.m., Ben Mahler wrote: > > src/slave/containerizer/provisioner/docker/registry_client.cpp, lines 63-66 > > <https://reviews.apache.org/r/38579/diff/5/?file=1088842#file1088842line63> > > > > Hm.. why did you need this additional static create? Looking at the > > implementation it seems as though you only need `RegistryClient::create`. Not sure I follow. This create is the factory for the process class. If there is an error creating the Process class, this should help in catching that. Isnt it? > On Oct. 2, 2015, 11:16 p.m., Ben Mahler wrote: > > src/slave/containerizer/provisioner/docker/registry_client.cpp, line 197 > > <https://reviews.apache.org/r/38579/diff/5/?file=1088842#file1088842line197> > > > > We try to avoid abbreviations for names like this, can you do a pass to > > clean them up? I have taken liberty when using commonly used variable names. creds, params etc are example of that (http://lxr.free-electrons.com/source/include/linux/sched.h). I can change it if you feel strongly about it. Also, in the context of Docker its always authentication. > On Oct. 2, 2015, 11:16 p.m., Ben Mahler wrote: > > src/slave/containerizer/provisioner/docker/registry_client.cpp, line 594 > > <https://reviews.apache.org/r/38579/diff/5/?file=1088842#file1088842line594> > > > > 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? i would have but i need the index number. > On Oct. 2, 2015, 11:16 p.m., Ben Mahler wrote: > > src/slave/containerizer/provisioner/docker/registry_client.cpp, lines > > 548-549 > > <https://reviews.apache.org/r/38579/diff/5/?file=1088842#file1088842line548> > > > > Why is this a member function? Can it be a standalone function for > > converting a response into a manifest? should i leave it as a lambda then? > On Oct. 2, 2015, 11:16 p.m., Ben Mahler wrote: > > src/slave/containerizer/provisioner/docker/registry_client.cpp, lines > > 716-718 > > <https://reviews.apache.org/r/38579/diff/5/?file=1088842#file1088842line716> > > > > Why only this validation? Should 'path' be a 'Path'? No since this is not a filepath but a url path > On Oct. 2, 2015, 11:16 p.m., Ben Mahler wrote: > > src/slave/containerizer/provisioner/docker/registry_client.cpp, lines > > 724-741 > > <https://reviews.apache.org/r/38579/diff/5/?file=1088842#file1088842line724> > > > > Is it acceptable to hold the whole blob in memory like this?? How big > > can these blobs be? Thats a good point. Ideally we should have a buffered socker reader. Would appreciate if you could point me to an example of buffered reader from http. - Jojy ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/38579/#review101382 ----------------------------------------------------------- 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 > >
