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

[Jojy Varghese]

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

[Timothy Chen]

> On Sept. 14, 2015, 6:11 p.m., Anand Mazumdar wrote:
> > src/slave/containerizer/provisioners/docker/registry_client.cpp, line 277
> > 
> >
> > 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
> > 
> >
> > 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
> > 
> >
> > 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.

[Anand Mazumdar]

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


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)


Not yours , but can we just do OK().status ?



src/slave/containerizer/provisioners/docker/registry_client.cpp (line 268)


Can we just do BadRequest().status here to eliminate the hard-coded string 
constant  ?



src/slave/containerizer/provisioners/docker/registry_client.cpp (line 277)


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)


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.

[Jojy Varghese]

> On Sept. 14, 2015, 6:11 p.m., Anand Mazumdar wrote:
> > src/slave/containerizer/provisioners/docker/registry_client.cpp, line 266
> > 
> >
> > 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.

[Anand Mazumdar]

> On Sept. 14, 2015, 6:11 p.m., Anand Mazumdar wrote:
> > src/slave/containerizer/provisioners/docker/registry_client.cpp, line 266
> > 
> >
> > 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.

[Jojy Varghese]

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


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



src/slave/containerizer/provisioners/docker/registry_client.cpp (line 404)


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.

[Timothy Chen]

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

[Timothy Chen]

> On Sept. 11, 2015, 4:57 a.m., Jojy Varghese wrote:
> > src/slave/containerizer/provisioners/docker/registry_client.cpp, line 386
> > 
> >
> > 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.

[Mesos ReviewBot]

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

[Mesos ReviewBot]

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

[Jojy Varghese]

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


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

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

[Timothy Chen]

> On Sept. 11, 2015, 4:57 a.m., Jojy Varghese wrote:
> > src/slave/containerizer/provisioners/docker/registry_client.cpp, line 572
> > 
> >
> > 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.

[Timothy Chen]

> On Sept. 11, 2015, 4:57 a.m., Jojy Varghese wrote:
> > src/slave/containerizer/provisioners/docker/registry_client.cpp, line 386
> > 
> >
> > 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.

[Jojy Varghese]

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


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)


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