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