> On Sept. 14, 2015, 6:11 p.m., Anand Mazumdar wrote:
> > src/slave/containerizer/provisioners/docker/registry_client.cpp, line 277
> > <https://reviews.apache.org/r/38289/diff/2/?file=1068777#file1068777line277>
> >
> >     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 ?

I think this is valid, but at the same time the JSON text is fairly verbose I'm 
was not inclined to print the whole JSON text in the slave log. The message 
according to the spec actually holds all the details we need to print.
I think you do bring some great points, that we're tied with the spec and we'll 
need to revisit as time goes on.
I think for now let's keep this as we're already tied with the spec, and we can 
change things moving forward as this is just one of the vey first patch of the 
client.


> On Sept. 14, 2015, 6:11 p.m., Anand Mazumdar wrote:
> > src/slave/containerizer/provisioners/docker/registry_client.cpp, line 268
> > <https://reviews.apache.org/r/38289/diff/2/?file=1068777#file1068777line268>
> >
> >     Can we just do BadRequest().status here to eliminate the hard-coded 
> > string constant  ?

Good idea, I was wondering if we had to do this. I'll do this another review.


> On Sept. 14, 2015, 6:11 p.m., Anand Mazumdar wrote:
> > src/Makefile.am, line 1694
> > <https://reviews.apache.org/r/38289/diff/2/?file=1068775#file1068775line1694>
> >
> >     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 ?

Sorry it's already merged :)


- Timothy


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


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

Reply via email to