You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Timothy Chen <tn...@apache.org> on 2015/09/11 05:58:03 UTC

Review Request 38289: Handle bad request in Docker registry client.

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

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

Re: Review Request 38289: Handle bad request in Docker registry client.

Posted by Timothy Chen <tn...@apache.org>.

> On Sept. 11, 2015, 4:57 a.m., Jojy Varghese wrote:
> > src/slave/containerizer/provisioners/docker/registry_client.cpp, line 572
> > <https://reviews.apache.org/r/38289/diff/1/?file=1067993#file1067993line572>
> >
> >     You will have to set O_NONBLOCK on the file descriptor.

This is done in io::write already.


- Timothy


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


On Sept. 11, 2015, 3:58 a.m., Timothy Chen wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38289/
> -----------------------------------------------------------
> 
> (Updated Sept. 11, 2015, 3:58 a.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
> 
>

Re: Review Request 38289: Handle bad request in Docker registry client.

Posted by Timothy Chen <tn...@apache.org>.

> On Sept. 11, 2015, 4:57 a.m., Jojy Varghese wrote:
> > src/slave/containerizer/provisioners/docker/registry_client.cpp, line 386
> > <https://reviews.apache.org/r/38289/diff/1/?file=1067993#file1067993line386>
> >
> >     This block should be refactored out so that it can be handled in line 278 also.
> 
> Timothy Chen wrote:
>     Not sure what you mean, this is 278:
>     
>           if (!resend) {
>             return Failure("Bad response: " + httpResponse.status);
>           }
>           
>     A Bad request shouldn't trigger a resend right?

Ah I see now, I'm moving it to the top since I think we should abort on any 400.


- Timothy


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


On Sept. 11, 2015, 3:58 a.m., Timothy Chen wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38289/
> -----------------------------------------------------------
> 
> (Updated Sept. 11, 2015, 3:58 a.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
> 
>

Re: Review Request 38289: Handle bad request in Docker registry client.

Posted by Timothy Chen <tn...@apache.org>.

> On Sept. 11, 2015, 4:57 a.m., Jojy Varghese wrote:
> > src/slave/containerizer/provisioners/docker/registry_client.cpp, line 386
> > <https://reviews.apache.org/r/38289/diff/1/?file=1067993#file1067993line386>
> >
> >     This block should be refactored out so that it can be handled in line 278 also.

Not sure what you mean, this is 278:

      if (!resend) {
        return Failure("Bad response: " + httpResponse.status);
      }
      
A Bad request shouldn't trigger a resend right?


- Timothy


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


On Sept. 11, 2015, 3:58 a.m., Timothy Chen wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38289/
> -----------------------------------------------------------
> 
> (Updated Sept. 11, 2015, 3:58 a.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
> 
>

Re: Review Request 38289: Handle bad request in Docker registry client.

Posted by Jojy Varghese <jo...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38289/#review98553
-----------------------------------------------------------



src/slave/containerizer/provisioners/docker/registry_client.cpp (line 386)
<https://reviews.apache.org/r/38289/#comment155062>

    This block should be refactored out so that it can be handled in line 278 also.



src/slave/containerizer/provisioners/docker/registry_client.cpp (line 563)
<https://reviews.apache.org/r/38289/#comment155063>

    You will have to set O_NONBLOCK on the file descriptor.


- Jojy Varghese


On Sept. 11, 2015, 3:58 a.m., Timothy Chen wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38289/
> -----------------------------------------------------------
> 
> (Updated Sept. 11, 2015, 3:58 a.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
> 
>

Re: Review Request 38289: Handle bad request in Docker registry client.

Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38289/#review98722
-----------------------------------------------------------


Bad patch!

Reviews applied: [37871, 37427, 37773, 38289]

Failed command: ./support/apply-review.sh -n -r 38289

Error:
 2015-09-12 01:22:41 URL:https://reviews.apache.org/r/38289/diff/raw/ [10197/10197] -> "38289.patch" [1]
error: patch failed: src/slave/containerizer/provisioners/docker/registry_client.hpp:54
error: src/slave/containerizer/provisioners/docker/registry_client.hpp: patch does not apply
Failed to apply patch

- Mesos ReviewBot


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

Re: Review Request 38289: Handle bad request in Docker registry client.

Posted by Jojy Varghese <jo...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38289/#review98872
-----------------------------------------------------------

Ship it!


Ship It!

- Jojy Varghese


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

Re: Review Request 38289: Handle bad request in Docker registry client.

Posted by Timothy Chen <tn...@apache.org>.

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

Re: Review Request 38289: Handle bad request in Docker registry client.

Posted by Anand Mazumdar <ma...@gmail.com>.

> On Sept. 14, 2015, 6:11 p.m., Anand Mazumdar wrote:
> > src/slave/containerizer/provisioners/docker/registry_client.cpp, line 266
> > <https://reviews.apache.org/r/38289/diff/2/?file=1068777#file1068777line266>
> >
> >     Not yours , but can we just do OK().status ?
> 
> Jojy Varghese wrote:
>     never understood the reasoning behind the construct. "OK().status" . I could imagine a const static like Http::OK_STATUS OR a response.isOk(). "OK().status" makes you create a new struct and then do a string comparision. We already have a response object and we should be asing it if it is an OK.

I think the reasoning would be more around `tech-debt` then anything else. I think we should create a JIRA if none exists to clean up the status hard-coded strings, define them as constants and then make the OK, BadRequest etc structs use these constants instead of duplicating them again in the corresponding cpp file.

https://github.com/apache/mesos/blob/master/3rdparty/libprocess/src/http.cpp#L77

There might be other alternative approaches too as you mentioned around `response.isOk()` etc . But if we have to go down any of them, we should alteast strive not to introduce any more "magic" constants , thereby, making this easier when we decide to reduce the debt.


- Anand


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

Re: Review Request 38289: Handle bad request in Docker registry client.

Posted by Jojy Varghese <jo...@mesosphere.io>.

> On Sept. 14, 2015, 6:11 p.m., Anand Mazumdar wrote:
> > src/slave/containerizer/provisioners/docker/registry_client.cpp, line 266
> > <https://reviews.apache.org/r/38289/diff/2/?file=1068777#file1068777line266>
> >
> >     Not yours , but can we just do OK().status ?

never understood the reasoning behind the construct. "OK().status" . I could imagine a const static like Http::OK_STATUS OR a response.isOk(). "OK().status" makes you create a new struct and then do a string comparision. We already have a response object and we should be asing it if it is an OK.


- Jojy


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

Re: Review Request 38289: Handle bad request in Docker registry client.

Posted by Anand Mazumdar <ma...@gmail.com>.
-----------------------------------------------------------
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
> 
>

Re: Review Request 38289: Handle bad request in Docker registry client.

Posted by Timothy Chen <tn...@apache.org>.
-----------------------------------------------------------
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 (updated)
-----

  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

Re: Review Request 38289: Handle bad request in Docker registry client.

Posted by Jojy Varghese <jo...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38289/#review98643
-----------------------------------------------------------



src/slave/containerizer/provisioners/docker/registry_client.cpp (line 391)
<https://reviews.apache.org/r/38289/#comment155153>

    change to standard error format "Failed to...
    " ?



src/slave/containerizer/provisioners/docker/registry_client.cpp (line 404)
<https://reviews.apache.org/r/38289/#comment155154>

    Maybe quote  "Errors" ?


- Jojy Varghese


On Sept. 11, 2015, 3:58 a.m., Timothy Chen wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38289/
> -----------------------------------------------------------
> 
> (Updated Sept. 11, 2015, 3:58 a.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
> 
>

Re: Review Request 38289: Handle bad request in Docker registry client.

Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38289/#review98593
-----------------------------------------------------------


Bad patch!

Reviews applied: [37871, 37427, 37773, 38289]

Failed command: ./support/apply-review.sh -n -r 38289

Error:
 2015-09-11 12:30:31 URL:https://reviews.apache.org/r/38289/diff/raw/ [10188/10188] -> "38289.patch" [1]
error: patch failed: src/slave/containerizer/provisioners/docker/registry_client.hpp:54
error: src/slave/containerizer/provisioners/docker/registry_client.hpp: patch does not apply
Failed to apply patch

- Mesos ReviewBot


On Sept. 11, 2015, 3:58 a.m., Timothy Chen wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38289/
> -----------------------------------------------------------
> 
> (Updated Sept. 11, 2015, 3:58 a.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
> 
>

Re: Review Request 38289: Handle bad request in Docker registry client.

Posted by Jojy Varghese <jo...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38289/#review98608
-----------------------------------------------------------



src/slave/containerizer/provisioners/docker/registry_client.cpp (line 386)
<https://reviews.apache.org/r/38289/#comment155132>

    block at line 278 will be executed when we expect a 200 but we get anything else. So for example the following flow - 
    - Request for manifest
    - Unauth
    - Send token
    - Temporary redirect
    - 400


- Jojy Varghese


On Sept. 11, 2015, 3:58 a.m., Timothy Chen wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38289/
> -----------------------------------------------------------
> 
> (Updated Sept. 11, 2015, 3:58 a.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
> 
>