mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Timothy Chen" <tnac...@apache.org>
Subject Re: Review Request 38289: Handle bad request in Docker registry client.
Date Mon, 14 Sep 2015 20:39:19 GMT


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


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