mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Jojy Varghese" <j...@mesosphere.io>
Subject Re: Review Request 37773: Docker: Adding registry client.
Date Tue, 01 Sep 2015 22:24:42 GMT


> On Sept. 1, 2015, 9:35 p.m., Timothy Chen wrote:
> > src/slave/containerizer/provisioners/docker/registry_client.cpp, line 186
> > <https://reviews.apache.org/r/37773/diff/9/?file=1060828#file1060828line186>
> >
> >     What's the benefits for this inline lambda vs just putting this code in the
foreach loop?
> >     
> >     I don't see you reuse it mulitple places and there is only one single call site.

its more functional to do it this way.


> On Sept. 1, 2015, 9:35 p.m., Timothy Chen wrote:
> > src/slave/containerizer/provisioners/docker/registry_client.cpp, line 214
> > <https://reviews.apache.org/r/37773/diff/9/?file=1060828#file1060828line214>
> >
> >     Can you add some verbose logging especially when we're calling ourselves again?
> >     
> >     This seems like code that we can get into trouble especially when the docker
registry implementation changes (ie: they start returning 202 instead of 200, or infinite
recirusion starts happening, etc).

We only explicitly handle certain status codes. We can expand the logci if required but currently
its very strict. Also, how can it get into infinite recursion?


> On Sept. 1, 2015, 9:35 p.m., Timothy Chen wrote:
> > src/slave/containerizer/provisioners/docker/registry_client.cpp, line 233
> > <https://reviews.apache.org/r/37773/diff/9/?file=1060828#file1060828line233>
> >
> >     You should state the assumption in a resend that we don't expect a response
status that causes another resend, which can still cause infinitte recursion.

Could you explain the case where it can cause infinite recursion?


> On Sept. 1, 2015, 9:35 p.m., Timothy Chen wrote:
> > src/slave/containerizer/provisioners/docker/registry_client.cpp, line 359
> > <https://reviews.apache.org/r/37773/diff/9/?file=1060828#file1060828line359>
> >
> >     Should we make sure somewhere that we encode or check the tag so that we don't
contain spaces?

I am not sure if http path can contain spaces. Queries can.


- Jojy


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


On Sept. 1, 2015, 9:36 p.m., Jojy Varghese wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37773/
> -----------------------------------------------------------
> 
> (Updated Sept. 1, 2015, 9:36 p.m.)
> 
> 
> Review request for mesos, Lily Chen, Joris Van Remoortere, and Timothy Chen.
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Added implementation for docker registry's Get Manifest and Get Blob APIs.
> 
> 
> Diffs
> -----
> 
>   src/Makefile.am 7b4d9f65506e7fa8425966009401aae73cdb79a5 
>   src/slave/containerizer/provisioners/docker/registry_client.hpp PRE-CREATION 
>   src/slave/containerizer/provisioners/docker/registry_client.cpp PRE-CREATION 
>   src/tests/provisioners/docker_provisioner_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/37773/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>


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