----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37773/#review97331 -----------------------------------------------------------
src/slave/containerizer/provisioners/docker/registry_client.hpp (line 34) <https://reviews.apache.org/r/37773/#comment153180> Remove provisioners namespace for now src/slave/containerizer/provisioners/docker/registry_client.hpp (line 34) <https://reviews.apache.org/r/37773/#comment153181> Remove provisioners namespace for now src/slave/containerizer/provisioners/docker/registry_client.cpp (line 50) <https://reviews.apache.org/r/37773/#comment153186> spell out credentials src/slave/containerizer/provisioners/docker/registry_client.cpp (line 67) <https://reviews.apache.org/r/37773/#comment153187> ditto src/slave/containerizer/provisioners/docker/registry_client.cpp (line 93) <https://reviews.apache.org/r/37773/#comment153188> fix ident (4 spaces, so move 2 spaces left) src/slave/containerizer/provisioners/docker/registry_client.cpp (line 108) <https://reviews.apache.org/r/37773/#comment153189> We should move all the logic into process, see similiar comment made in AppProvisionerProcess (https://reviews.apache.org/r/37881/diff/2?file=1059946#file1059946line174) src/slave/containerizer/provisioners/docker/registry_client.cpp (line 186) <https://reviews.apache.org/r/37773/#comment153183> 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. src/slave/containerizer/provisioners/docker/registry_client.cpp (line 214) <https://reviews.apache.org/r/37773/#comment153201> 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). src/slave/containerizer/provisioners/docker/registry_client.cpp (line 219) <https://reviews.apache.org/r/37773/#comment153190> This should be Option<string> right? Since we might not have a lastResponseStatus. And we can default it to None() so all the callers for this method can just skip that instead of passing in a "" Can you also comment on why we need this? src/slave/containerizer/provisioners/docker/registry_client.cpp (line 233) <https://reviews.apache.org/r/37773/#comment153191> 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. src/slave/containerizer/provisioners/docker/registry_client.cpp (line 235) <https://reviews.apache.org/r/37773/#comment153200> You did "Invalid Response :" + error in the bottom, and "Invalid Response '" + error + "'" here. Let's just keep the same format, I suggest the former. Also I think worth mentioning that this is matching the last response. src/slave/containerizer/provisioners/docker/registry_client.cpp (line 266) <https://reviews.apache.org/r/37773/#comment153192> The identation seems off, I think you should probably just move the self() to the last line and the beginning of lambda too. src/slave/containerizer/provisioners/docker/registry_client.cpp (line 281) <https://reviews.apache.org/r/37773/#comment153193> Can you also add a comment that we need to add redirect functionality into libprocess too? Once we have that we don't have to handle it ourselves src/slave/containerizer/provisioners/docker/registry_client.cpp (line 291) <https://reviews.apache.org/r/37773/#comment153195> strings::contains src/slave/containerizer/provisioners/docker/registry_client.cpp (line 293) <https://reviews.apache.org/r/37773/#comment153194> Fix alignment src/slave/containerizer/provisioners/docker/registry_client.cpp (line 306) <https://reviews.apache.org/r/37773/#comment153196> Put this in a constant. src/slave/containerizer/provisioners/docker/registry_client.cpp (line 309) <https://reviews.apache.org/r/37773/#comment153197> Can you comment what's this block is for? src/slave/containerizer/provisioners/docker/registry_client.cpp (line 315) <https://reviews.apache.org/r/37773/#comment153198> Fix indentation. src/slave/containerizer/provisioners/docker/registry_client.cpp (line 336) <https://reviews.apache.org/r/37773/#comment153199> Fix alignment src/slave/containerizer/provisioners/docker/registry_client.cpp (line 359) <https://reviews.apache.org/r/37773/#comment153204> Should we make sure somewhere that we encode or check the tag so that we don't contain spaces? src/slave/containerizer/provisioners/docker/registry_client.cpp (line 382) <https://reviews.apache.org/r/37773/#comment153202> Move this to previous line src/slave/containerizer/provisioners/docker/registry_client.cpp (line 401) <https://reviews.apache.org/r/37773/#comment153203> don't take reference for size_t, just pass by value src/slave/containerizer/provisioners/docker/registry_client.cpp (line 410) <https://reviews.apache.org/r/37773/#comment153208> I think ideally we can introduce something in libprocess so we can stream results to disk with splice or something like that, avoid as much copies as we can. src/tests/provisioners/docker_provisioner_tests.cpp (line 34) <https://reviews.apache.org/r/37773/#comment153209> This goes before token_manager src/tests/provisioners/docker_provisioner_tests.cpp (line 231) <https://reviews.apache.org/r/37773/#comment153213> extra space between if and ( src/tests/provisioners/docker_provisioner_tests.cpp (line 232) <https://reviews.apache.org/r/37773/#comment153214> Why is this needed? - Timothy Chen On Sept. 1, 2015, 12:02 a.m., Jojy Varghese wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/37773/ > ----------------------------------------------------------- > > (Updated Sept. 1, 2015, 12:02 a.m.) > > > Review request for mesos, Lily Chen 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 > >
