----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/38289/#review98876 -----------------------------------------------------------
LGTM. Just a minor query around why do we want to parse the error response JSON and not directly return the entire response JSON ? src/Makefile.am (line 1693) <https://reviews.apache.org/r/38289/#comment155521> Can we do this renaming/moving test files in a separate patch ? This looks un-related to handling BadRequest in the Registry Client. What do you think ? src/slave/containerizer/provisioners/docker/registry_client.cpp (line 266) <https://reviews.apache.org/r/38289/#comment155522> Not yours , but can we just do OK().status ? src/slave/containerizer/provisioners/docker/registry_client.cpp (line 268) <https://reviews.apache.org/r/38289/#comment155523> Can we just do BadRequest().status here to eliminate the hard-coded string constant ? src/slave/containerizer/provisioners/docker/registry_client.cpp (line 277) <https://reviews.apache.org/r/38289/#comment155543> Why are we parsing the error JSON to extract the error string from JSON here and not dump the entire error string to the end-user. Is the rationale that this makes the error messages readable and more consistent ? However, the cons to this are: 1. We don't seem to be following this design any-where else in our code-base ? 2. We have already omitted fields `code` and `detail` from the error response ( http://docs.docker.com/registry/spec/api/ ). If Docker adds more fields in the future or deletes/renames some of them, we would need to revisit them again? Won't providing the entire JSON to the end-user be more helpful in identifying the root-case and helping him/her resolve the issue faster ? src/slave/containerizer/provisioners/docker/registry_client.cpp (line 454) <https://reviews.apache.org/r/38289/#comment155547> Nit: 'Docker-Content-Digest' in quotes. - Anand Mazumdar On Sept. 11, 2015, 7:34 p.m., Timothy Chen wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/38289/ > ----------------------------------------------------------- > > (Updated Sept. 11, 2015, 7:34 p.m.) > > > Review request for mesos and Jojy Varghese. > > > Repository: mesos > > > Description > ------- > > Handle bad request in Docker registry client. > > > Diffs > ----- > > src/Makefile.am 8963cea9fd7e3bee450a1059e6383e5ab868a17b > 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 > ff29d562c7f1bd5f0579e97cdbd60c2b2f36329e > > Diff: https://reviews.apache.org/r/38289/diff/ > > > Testing > ------- > > make check > > > Thanks, > > Timothy Chen > >
