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

Reply via email to