Re: Review Request 37773: Docker: Adding registry client.

2015-09-10 Thread Mesos ReviewBot 

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


Patch looks great!

Reviews applied: [37871, 37427, 37773]

All tests passed.

- Mesos ReviewBot


On Sept. 10, 2015, 4:53 a.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37773/
> ---
> 
> (Updated Sept. 10, 2015, 4:53 a.m.)
> 
> 
> Review request for mesos, Lily Chen, Joris Van Remoortere, and Timothy Chen.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added implementation for docker registry's Get Manifest and Get Blob APIs.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am cea470e14ed93a46fead9da8015df676e818e4bc 
>   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/37773/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>

Re: Review Request 37773: Docker: Adding registry client.

2015-09-09 Thread Mesos ReviewBot 

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


Patch looks great!

Reviews applied: [37871, 37427, 37773]

All tests passed.

- Mesos ReviewBot


On Sept. 9, 2015, 8:08 p.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37773/
> ---
> 
> (Updated Sept. 9, 2015, 8:08 p.m.)
> 
> 
> Review request for mesos, Lily Chen, Joris Van Remoortere, and Timothy Chen.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added implementation for docker registry's Get Manifest and Get Blob APIs.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 4ef58cd89f4a92770a7e7a8624a7befc801c6d69 
>   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/37773/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>

Re: Review Request 37773: Docker: Adding registry client.

2015-09-09 Thread Jojy Varghese 

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

(Updated Sept. 9, 2015, 11:47 p.m.)


Review request for mesos, Lily Chen, Joris Van Remoortere, and Timothy Chen.


Changes
---

Added more info to manifest response


Repository: mesos


Description
---

Added implementation for docker registry's Get Manifest and Get Blob APIs.


Diffs (updated)
-

  docs/persistent-volume.md b5dd6d8ec68d8ed7dd4787ffc9973bbe6c49965a 
  src/Makefile.am 4ef58cd89f4a92770a7e7a8624a7befc801c6d69 
  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/37773/diff/


Testing
---

make check


Thanks,

Jojy Varghese

Re: Review Request 37773: Docker: Adding registry client.

2015-09-09 Thread Timothy Chen 

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



docs/persistent-volume.md (line 227)


?


- Timothy Chen


On Sept. 9, 2015, 11:47 p.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37773/
> ---
> 
> (Updated Sept. 9, 2015, 11:47 p.m.)
> 
> 
> Review request for mesos, Lily Chen, Joris Van Remoortere, and Timothy Chen.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added implementation for docker registry's Get Manifest and Get Blob APIs.
> 
> 
> Diffs
> -
> 
>   docs/persistent-volume.md b5dd6d8ec68d8ed7dd4787ffc9973bbe6c49965a 
>   src/Makefile.am 4ef58cd89f4a92770a7e7a8624a7befc801c6d69 
>   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/37773/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>

Re: Review Request 37773: Docker: Adding registry client.

2015-09-09 Thread Timothy Chen 

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



src/slave/containerizer/provisioners/docker/registry_client.hpp (line 57)


Mannifest -> Manifest



src/slave/containerizer/provisioners/docker/registry_client.hpp (line 81)


Remove extra space.


- Timothy Chen


On Sept. 9, 2015, 11:47 p.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37773/
> ---
> 
> (Updated Sept. 9, 2015, 11:47 p.m.)
> 
> 
> Review request for mesos, Lily Chen, Joris Van Remoortere, and Timothy Chen.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added implementation for docker registry's Get Manifest and Get Blob APIs.
> 
> 
> Diffs
> -
> 
>   docs/persistent-volume.md b5dd6d8ec68d8ed7dd4787ffc9973bbe6c49965a 
>   src/Makefile.am 4ef58cd89f4a92770a7e7a8624a7befc801c6d69 
>   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/37773/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>

Re: Review Request 37773: Docker: Adding registry client.

2015-09-09 Thread Mesos ReviewBot 

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


Patch looks great!

Reviews applied: [37871, 37427, 37773]

All tests passed.

- Mesos ReviewBot


On Sept. 9, 2015, 11:47 p.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37773/
> ---
> 
> (Updated Sept. 9, 2015, 11:47 p.m.)
> 
> 
> Review request for mesos, Lily Chen, Joris Van Remoortere, and Timothy Chen.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added implementation for docker registry's Get Manifest and Get Blob APIs.
> 
> 
> Diffs
> -
> 
>   docs/persistent-volume.md b5dd6d8ec68d8ed7dd4787ffc9973bbe6c49965a 
>   src/Makefile.am 4ef58cd89f4a92770a7e7a8624a7befc801c6d69 
>   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/37773/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>

Re: Review Request 37773: Docker: Adding registry client.

2015-09-09 Thread Jojy Varghese 

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

(Updated Sept. 10, 2015, 4:49 a.m.)


Review request for mesos, Lily Chen, Joris Van Remoortere, and Timothy Chen.


Changes
---

added more logs


Repository: mesos


Description
---

Added implementation for docker registry's Get Manifest and Get Blob APIs.


Diffs (updated)
-

  src/Makefile.am cea470e14ed93a46fead9da8015df676e818e4bc 
  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/37773/diff/


Testing
---

make check


Thanks,

Jojy Varghese

Re: Review Request 37773: Docker: Adding registry client.

2015-09-09 Thread Jojy Varghese 

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

(Updated Sept. 10, 2015, 4:53 a.m.)


Review request for mesos, Lily Chen, Joris Van Remoortere, and Timothy Chen.


Changes
---

removed extraneous change.


Repository: mesos


Description
---

Added implementation for docker registry's Get Manifest and Get Blob APIs.


Diffs (updated)
-

  src/Makefile.am cea470e14ed93a46fead9da8015df676e818e4bc 
  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/37773/diff/


Testing
---

make check


Thanks,

Jojy Varghese

Re: Review Request 37773: Docker: Adding registry client.

2015-09-09 Thread Jojy Varghese 

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

(Updated Sept. 9, 2015, 4:50 p.m.)


Review request for mesos, Lily Chen, Joris Van Remoortere, and Timothy Chen.


Changes
---

rebased with master.


Repository: mesos


Description
---

Added implementation for docker registry's Get Manifest and Get Blob APIs.


Diffs (updated)
-

  src/Makefile.am 0a8ef6d8551cf177cb565b2a443c05e8eea5ab1c 
  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 PRE-CREATION 

Diff: https://reviews.apache.org/r/37773/diff/


Testing
---

make check


Thanks,

Jojy Varghese

Re: Review Request 37773: Docker: Adding registry client.

2015-09-09 Thread Timothy Chen 


> On Sept. 1, 2015, 9:35 p.m., Timothy Chen wrote:
> > src/slave/containerizer/provisioners/docker/registry_client.cpp, line 359
> > 
> >
> > Should we make sure somewhere that we encode or check the tag so that 
> > we don't contain spaces?
> 
> Jojy Varghese wrote:
> I am not sure if http path can contain spaces. Queries can.

Therefore we shouldn't allow it right?


- Timothy


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


On Sept. 9, 2015, 4:50 p.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37773/
> ---
> 
> (Updated Sept. 9, 2015, 4:50 p.m.)
> 
> 
> Review request for mesos, Lily Chen, Joris Van Remoortere, and Timothy Chen.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added implementation for docker registry's Get Manifest and Get Blob APIs.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 0a8ef6d8551cf177cb565b2a443c05e8eea5ab1c 
>   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 PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/37773/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>

Re: Review Request 37773: Docker: Adding registry client.

2015-09-09 Thread Jojy Varghese 

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

(Updated Sept. 9, 2015, 8:08 p.m.)


Review request for mesos, Lily Chen, Joris Van Remoortere, and Timothy Chen.


Changes
---

rebased with master. addressed review comments.


Repository: mesos


Description
---

Added implementation for docker registry's Get Manifest and Get Blob APIs.


Diffs (updated)
-

  src/Makefile.am 4ef58cd89f4a92770a7e7a8624a7befc801c6d69 
  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/37773/diff/


Testing
---

make check


Thanks,

Jojy Varghese

Re: Review Request 37773: Docker: Adding registry client.

2015-09-09 Thread Jojy Varghese 


> On Sept. 1, 2015, 9:35 p.m., Timothy Chen wrote:
> > src/slave/containerizer/provisioners/docker/registry_client.cpp, line 359
> > 
> >
> > Should we make sure somewhere that we encode or check the tag so that 
> > we don't contain spaces?
> 
> Jojy Varghese wrote:
> I am not sure if http path can contain spaces. Queries can.
> 
> Timothy Chen wrote:
> Therefore we shouldn't allow it right?

Ok will add validation. Although this validation belongs in the URL class.


- Jojy


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


On Sept. 9, 2015, 4:50 p.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37773/
> ---
> 
> (Updated Sept. 9, 2015, 4:50 p.m.)
> 
> 
> Review request for mesos, Lily Chen, Joris Van Remoortere, and Timothy Chen.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added implementation for docker registry's Get Manifest and Get Blob APIs.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 0a8ef6d8551cf177cb565b2a443c05e8eea5ab1c 
>   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 PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/37773/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>

Re: Review Request 37773: Docker: Adding registry client.

2015-09-02 Thread Jojy Varghese 

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

(Updated Sept. 2, 2015, 5:06 p.m.)


Review request for mesos, Lily Chen, Joris Van Remoortere, and Timothy Chen.


Changes
---

review comments addressed.


Repository: mesos


Description
---

Added implementation for docker registry's Get Manifest and Get Blob APIs.


Diffs (updated)
-

  src/Makefile.am 7b4d9f65506e7fa8425966009401aae73cdb79a5 
  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 PRE-CREATION 

Diff: https://reviews.apache.org/r/37773/diff/


Testing
---

make check


Thanks,

Jojy Varghese

Re: Review Request 37773: Docker: Adding registry client.

2015-09-02 Thread Jojy Varghese 


- Jojy


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


On Sept. 1, 2015, 9:36 p.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37773/
> ---
> 
> (Updated Sept. 1, 2015, 9:36 p.m.)
> 
> 
> Review request for mesos, Lily Chen, Joris Van Remoortere, and Timothy Chen.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added implementation for docker registry's Get Manifest and Get Blob APIs.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 7b4d9f65506e7fa8425966009401aae73cdb79a5 
>   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 PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/37773/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>

Re: Review Request 37773: Docker: Adding registry client.

2015-09-02 Thread Mesos ReviewBot 

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


Patch looks great!

Reviews applied: [37871, 37427, 37773]

All tests passed.

- Mesos ReviewBot


On Sept. 2, 2015, 5:06 p.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37773/
> ---
> 
> (Updated Sept. 2, 2015, 5:06 p.m.)
> 
> 
> Review request for mesos, Lily Chen, Joris Van Remoortere, and Timothy Chen.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added implementation for docker registry's Get Manifest and Get Blob APIs.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 7b4d9f65506e7fa8425966009401aae73cdb79a5 
>   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 PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/37773/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>

Re: Review Request 37773: Docker: Adding registry client.

2015-09-02 Thread Jojy Varghese 

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

(Updated Sept. 2, 2015, 10:52 p.m.)


Review request for mesos, Lily Chen, Joris Van Remoortere, and Timothy Chen.


Changes
---

rebased with dependent patch.


Repository: mesos


Description
---

Added implementation for docker registry's Get Manifest and Get Blob APIs.


Diffs (updated)
-

  src/Makefile.am 7b4d9f65506e7fa8425966009401aae73cdb79a5 
  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 PRE-CREATION 

Diff: https://reviews.apache.org/r/37773/diff/


Testing
---

make check


Thanks,

Jojy Varghese

Re: Review Request 37773: Docker: Adding registry client.

2015-09-01 Thread Jojy Varghese 


> On Sept. 1, 2015, 9:35 p.m., Timothy Chen wrote:
> > src/tests/provisioners/docker_provisioner_tests.cpp, line 232
> > 
> >
> > Why is this needed?

So that we cleanup temporaries created in the SSLTest setup.


- Jojy


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


On Sept. 1, 2015, 9:36 p.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37773/
> ---
> 
> (Updated Sept. 1, 2015, 9:36 p.m.)
> 
> 
> Review request for mesos, Lily Chen, Joris Van Remoortere, and Timothy Chen.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added implementation for docker registry's Get Manifest and Get Blob APIs.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 7b4d9f65506e7fa8425966009401aae73cdb79a5 
>   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 PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/37773/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>

Re: Review Request 37773: Docker: Adding registry client.

2015-09-01 Thread Jojy Varghese 


> On Sept. 1, 2015, 9:35 p.m., Timothy Chen wrote:
> > src/slave/containerizer/provisioners/docker/registry_client.cpp, line 186
> > 
> >
> > What's the benefits for this inline lambda vs just putting this code in 
> > the foreach loop?
> > 
> > I don't see you reuse it mulitple places and there is only one single 
> > call site.

its more functional to do it this way.


> On Sept. 1, 2015, 9:35 p.m., Timothy Chen wrote:
> > src/slave/containerizer/provisioners/docker/registry_client.cpp, line 214
> > 
> >
> > Can you add some verbose logging especially when we're calling 
> > ourselves again?
> > 
> > This seems like code that we can get into trouble especially when the 
> > docker registry implementation changes (ie: they start returning 202 
> > instead of 200, or infinite recirusion starts happening, etc).

We only explicitly handle certain status codes. We can expand the logci if 
required but currently its very strict. Also, how can it get into infinite 
recursion?


> On Sept. 1, 2015, 9:35 p.m., Timothy Chen wrote:
> > src/slave/containerizer/provisioners/docker/registry_client.cpp, line 233
> > 
> >
> > You should state the assumption in a resend that we don't expect a 
> > response status that causes another resend, which can still cause infinitte 
> > recursion.

Could you explain the case where it can cause infinite recursion?


> On Sept. 1, 2015, 9:35 p.m., Timothy Chen wrote:
> > src/slave/containerizer/provisioners/docker/registry_client.cpp, line 359
> > 
> >
> > Should we make sure somewhere that we encode or check the tag so that 
> > we don't contain spaces?

I am not sure if http path can contain spaces. Queries can.


- Jojy


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


On Sept. 1, 2015, 9:36 p.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37773/
> ---
> 
> (Updated Sept. 1, 2015, 9:36 p.m.)
> 
> 
> Review request for mesos, Lily Chen, Joris Van Remoortere, and Timothy Chen.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added implementation for docker registry's Get Manifest and Get Blob APIs.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 7b4d9f65506e7fa8425966009401aae73cdb79a5 
>   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 PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/37773/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>

Re: Review Request 37773: Docker: Adding registry client.

2015-09-01 Thread Jojy Varghese 

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



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


Why add the heavy lifting of process dispatch when we can avoid it? Wont be 
efficient.



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


Will exceed 80 chars.



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


I can add a todo/jira.


- Jojy Varghese


On Sept. 1, 2015, 9:36 p.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37773/
> ---
> 
> (Updated Sept. 1, 2015, 9:36 p.m.)
> 
> 
> Review request for mesos, Lily Chen, Joris Van Remoortere, and Timothy Chen.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added implementation for docker registry's Get Manifest and Get Blob APIs.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 7b4d9f65506e7fa8425966009401aae73cdb79a5 
>   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 PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/37773/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>

Re: Review Request 37773: Docker: Adding registry client.

2015-09-01 Thread Timothy Chen 

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



src/slave/containerizer/provisioners/docker/registry_client.hpp (line 34)


Remove provisioners namespace for now



src/slave/containerizer/provisioners/docker/registry_client.hpp (line 34)


Remove provisioners namespace for now



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


spell out credentials



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


ditto



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


fix ident (4 spaces, so move 2 spaces left)



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


We should move all the logic into process, see similiar comment made in 
AppProvisionerProcess 
(https://reviews.apache.org/r/37881/diff/2?file=1059946#file1059946line174)



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


What's the benefits for this inline lambda vs just putting this code in the 
foreach loop?

I don't see you reuse it mulitple places and there is only one single call 
site.



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


Can you add some verbose logging especially when we're calling ourselves 
again?

This seems like code that we can get into trouble especially when the 
docker registry implementation changes (ie: they start returning 202 instead of 
200, or infinite recirusion starts happening, etc).



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


This should be Option right? Since we might not have a 
lastResponseStatus. And we can default it to None() so all the callers for this 
method can just skip that instead of passing in a ""

Can you also comment on why we need this?



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


You should state the assumption in a resend that we don't expect a response 
status that causes another resend, which can still cause infinitte recursion.



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


You did "Invalid Response :" + error in the bottom, and "Invalid Response 
'" + error + "'" here.

Let's just keep the same format, I suggest the former.

Also I think worth mentioning that this is matching the last response.



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


The identation seems off, I think you should probably just move the self() 
to the last line and the beginning of lambda too.



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


Can you also add a comment that we need to add redirect functionality into 
libprocess too? Once we have that we don't have to handle it ourselves



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


strings::contains



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


Fix alignment



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


Put this in a constant.



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


Can you comment what's this block is for?



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


Fix indentation.



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


Fix alignment



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


Should we make sure somewhere that we encode or check the tag so that we 
don't contain spaces?



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


Move

Re: Review Request 37773: Docker: Adding registry client.

2015-09-01 Thread Jojy Varghese 


> On Sept. 1, 2015, 9:35 p.m., Timothy Chen wrote:
> > src/slave/containerizer/provisioners/docker/registry_client.cpp, line 410
> > 
> >
> > I think ideally we can introduce something in libprocess so we can 
> > stream results to disk with splice or something like that, avoid as much 
> > copies as we can.

yeah would be nice to have such functionality in libproces.


> On Sept. 1, 2015, 9:35 p.m., Timothy Chen wrote:
> > src/slave/containerizer/provisioners/docker/registry_client.cpp, line 306
> > 
> >
> > Put this in a constant.

the constant should ideally be in http namespace but there we use literals.


- Jojy


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


On Sept. 1, 2015, 9:36 p.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37773/
> ---
> 
> (Updated Sept. 1, 2015, 9:36 p.m.)
> 
> 
> Review request for mesos, Lily Chen, Joris Van Remoortere, and Timothy Chen.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added implementation for docker registry's Get Manifest and Get Blob APIs.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 7b4d9f65506e7fa8425966009401aae73cdb79a5 
>   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 PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/37773/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>

Re: Review Request 37773: Docker: Adding registry client.

2015-08-31 Thread Jojy Varghese 


> On Aug. 31, 2015, 8:42 p.m., Timothy Chen wrote:
> > src/slave/containerizer/provisioners/docker/registry_client.cpp, line 456
> > 
> >
> > is maxSize being used at all? If not we should remove all references of 
> > it.

I added it as part of the external facing interface so that it doesnt change 
when we add validation of max size.


- Jojy


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


On Aug. 30, 2015, 3:12 p.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37773/
> ---
> 
> (Updated Aug. 30, 2015, 3:12 p.m.)
> 
> 
> Review request for mesos, Lily Chen and Timothy Chen.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added implementation for docker registry's Get Manifest and Get Blob APIs.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 7b620ff66856b3f0adac121b3297d55ed71a3d99 
>   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 PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/37773/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>

Re: Review Request 37773: Docker: Adding registry client.

2015-08-28 Thread Timothy Chen 

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



src/slave/containerizer/provisioners/docker/registry_client.hpp (line 46)
https://reviews.apache.org/r/37773/#comment152585

Why not just use option.getOrElse()?



src/slave/containerizer/provisioners/docker/registry_client.hpp (line 77)
https://reviews.apache.org/r/37773/#comment152586

userId



src/slave/containerizer/provisioners/docker/registry_client.hpp (line 116)
https://reviews.apache.org/r/37773/#comment152587

We usually spell timeout as one word everywhere else, so all lower case.



src/slave/containerizer/provisioners/docker/registry_client.hpp (line 150)
https://reviews.apache.org/r/37773/#comment152588

Do we need to define static here if it's not being used anywhere else but 
the cpp files?



src/slave/containerizer/provisioners/docker/registry_client.cpp (line 41)
https://reviews.apache.org/r/37773/#comment152589

Kill extra line



src/slave/containerizer/provisioners/docker/registry_client.cpp (line 87)
https://reviews.apache.org/r/37773/#comment152593

What's the reasoning behind prefixing underscores for these? You've already 
added trailing underscores for your class variables right?



src/slave/containerizer/provisioners/docker/registry_client.cpp (line 91)
https://reviews.apache.org/r/37773/#comment152590

getOrElse



src/slave/containerizer/provisioners/docker/registry_client.cpp (line 114)
https://reviews.apache.org/r/37773/#comment152592

I'm wondering why you need to use promise here.

If you put all this logic in getManifest then essentially you just need to 
return the dispatch right? And in getManifest you just chian your return with 
onFailed and onAny too.

Also even if you don't move it in getManifest why can't you just return 
result.onFailed(...).onAny(...)?



src/slave/containerizer/provisioners/docker/registry_client.cpp (line 153)
https://reviews.apache.org/r/37773/#comment152594

What residue?


- Timothy Chen


On Aug. 28, 2015, 6:38 p.m., Jojy Varghese wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviews.apache.org/r/37773/
 ---
 
 (Updated Aug. 28, 2015, 6:38 p.m.)
 
 
 Review request for mesos, Lily Chen and Timothy Chen.
 
 
 Repository: mesos
 
 
 Description
 ---
 
 Added implementation for docker registry's Get Manifest and Get Blob APIs.
 
 
 Diffs
 -
 
   src/Makefile.am 7b620ff66856b3f0adac121b3297d55ed71a3d99 
   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 PRE-CREATION 
 
 Diff: https://reviews.apache.org/r/37773/diff/
 
 
 Testing
 ---
 
 make check
 
 
 Thanks,
 
 Jojy Varghese

Re: Review Request 37773: Docker: Adding registry client.

2015-08-28 Thread Jojy Varghese 

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

(Updated Aug. 28, 2015, 6:38 p.m.)


Review request for mesos, Lily Chen and Timothy Chen.


Changes
---

review comments addressed


Repository: mesos


Description
---

Added implementation for docker registry's Get Manifest and Get Blob APIs.


Diffs (updated)
-

  src/Makefile.am 7b620ff66856b3f0adac121b3297d55ed71a3d99 
  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 PRE-CREATION 

Diff: https://reviews.apache.org/r/37773/diff/


Testing
---

make check


Thanks,

Jojy Varghese

Re: Review Request 37773: Docker: Adding registry client.

2015-08-28 Thread Jojy Varghese 


 On Aug. 28, 2015, 5:21 p.m., Lily Chen wrote:
  src/slave/containerizer/provisioners/docker/registry_client.hpp, line 34
  https://reviews.apache.org/r/37773/diff/5/?file=1057183#file1057183line34
 
  No need for provisioners namespace, see appc example of 
  mesos::internal::slave::appc

Why is that?


- Jojy


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


On Aug. 28, 2015, 6:38 p.m., Jojy Varghese wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviews.apache.org/r/37773/
 ---
 
 (Updated Aug. 28, 2015, 6:38 p.m.)
 
 
 Review request for mesos, Lily Chen and Timothy Chen.
 
 
 Repository: mesos
 
 
 Description
 ---
 
 Added implementation for docker registry's Get Manifest and Get Blob APIs.
 
 
 Diffs
 -
 
   src/Makefile.am 7b620ff66856b3f0adac121b3297d55ed71a3d99 
   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 PRE-CREATION 
 
 Diff: https://reviews.apache.org/r/37773/diff/
 
 
 Testing
 ---
 
 make check
 
 
 Thanks,
 
 Jojy Varghese

Re: Review Request 37773: Docker: Adding registry client.

2015-08-28 Thread Lily Chen 

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



src/slave/containerizer/provisioners/docker/registry_client.cpp (line 409)
https://reviews.apache.org/r/37773/#comment152627

I think path may be misleading, can we name this repo?



src/slave/containerizer/provisioners/docker/registry_client.cpp (line 415)
https://reviews.apache.org/r/37773/#comment152624

can we do a path::join here? It would be hard to get the /s correct just 
passing path through



src/slave/containerizer/provisioners/docker/registry_client.cpp (line 465)
https://reviews.apache.org/r/37773/#comment152628

Same issue with path naming as with getManifest



src/slave/containerizer/provisioners/docker/registry_client.cpp (lines 468 - 
469)
https://reviews.apache.org/r/37773/#comment152632

Add defaults here as with as with image manifest.


- Lily Chen


On Aug. 28, 2015, 6:38 p.m., Jojy Varghese wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviews.apache.org/r/37773/
 ---
 
 (Updated Aug. 28, 2015, 6:38 p.m.)
 
 
 Review request for mesos, Lily Chen and Timothy Chen.
 
 
 Repository: mesos
 
 
 Description
 ---
 
 Added implementation for docker registry's Get Manifest and Get Blob APIs.
 
 
 Diffs
 -
 
   src/Makefile.am 7b620ff66856b3f0adac121b3297d55ed71a3d99 
   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 PRE-CREATION 
 
 Diff: https://reviews.apache.org/r/37773/diff/
 
 
 Testing
 ---
 
 make check
 
 
 Thanks,
 
 Jojy Varghese

Re: Review Request 37773: Docker: Adding registry client.

2015-08-28 Thread Jojy Varghese 


 On Aug. 28, 2015, 11:03 p.m., Lily Chen wrote:
  src/slave/containerizer/provisioners/docker/registry_client.cpp, line 415
  https://reviews.apache.org/r/37773/diff/6/?file=1058283#file1058283line415
 
  can we do a path::join here? It would be hard to get the /s correct 
  just passing path through

This is not a file path. So os::path wont work.


- Jojy


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


On Aug. 28, 2015, 6:38 p.m., Jojy Varghese wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviews.apache.org/r/37773/
 ---
 
 (Updated Aug. 28, 2015, 6:38 p.m.)
 
 
 Review request for mesos, Lily Chen and Timothy Chen.
 
 
 Repository: mesos
 
 
 Description
 ---
 
 Added implementation for docker registry's Get Manifest and Get Blob APIs.
 
 
 Diffs
 -
 
   src/Makefile.am 7b620ff66856b3f0adac121b3297d55ed71a3d99 
   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 PRE-CREATION 
 
 Diff: https://reviews.apache.org/r/37773/diff/
 
 
 Testing
 ---
 
 make check
 
 
 Thanks,
 
 Jojy Varghese

Re: Review Request 37773: Docker: Adding registry client.

2015-08-28 Thread Jojy Varghese 


 On Aug. 28, 2015, 11:03 p.m., Lily Chen wrote:
  src/slave/containerizer/provisioners/docker/registry_client.cpp, line 409
  https://reviews.apache.org/r/37773/diff/6/?file=1058283#file1058283line409
 
  I think path may be misleading, can we name this repo?

https://docs.docker.com/registry/spec/api/. If you look at the description of 
the field, its a path.


- Jojy


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


On Aug. 28, 2015, 6:38 p.m., Jojy Varghese wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviews.apache.org/r/37773/
 ---
 
 (Updated Aug. 28, 2015, 6:38 p.m.)
 
 
 Review request for mesos, Lily Chen and Timothy Chen.
 
 
 Repository: mesos
 
 
 Description
 ---
 
 Added implementation for docker registry's Get Manifest and Get Blob APIs.
 
 
 Diffs
 -
 
   src/Makefile.am 7b620ff66856b3f0adac121b3297d55ed71a3d99 
   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 PRE-CREATION 
 
 Diff: https://reviews.apache.org/r/37773/diff/
 
 
 Testing
 ---
 
 make check
 
 
 Thanks,
 
 Jojy Varghese

Re: Review Request 37773: Docker: Adding registry client.

2015-08-28 Thread Jojy Varghese 


 On Aug. 28, 2015, 6:46 p.m., Timothy Chen wrote:
  src/slave/containerizer/provisioners/docker/registry_client.hpp, line 150
  https://reviews.apache.org/r/37773/diff/6/?file=1058282#file1058282line150
 
  Do we need to define static here if it's not being used anywhere else 
  but the cpp files?

Its a class constant and hence declared here.


 On Aug. 28, 2015, 6:46 p.m., Timothy Chen wrote:
  src/slave/containerizer/provisioners/docker/registry_client.cpp, line 87
  https://reviews.apache.org/r/37773/diff/6/?file=1058283#file1058283line87
 
  What's the reasoning behind prefixing underscores for these? You've 
  already added trailing underscores for your class variables right?

The function body uses similar variables. Hence to disambiguate.


 On Aug. 28, 2015, 6:46 p.m., Timothy Chen wrote:
  src/slave/containerizer/provisioners/docker/registry_client.cpp, line 153
  https://reviews.apache.org/r/37773/diff/6/?file=1058283#file1058283line153
 
  What residue?

The new directory created.


- Jojy


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


On Aug. 28, 2015, 6:38 p.m., Jojy Varghese wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviews.apache.org/r/37773/
 ---
 
 (Updated Aug. 28, 2015, 6:38 p.m.)
 
 
 Review request for mesos, Lily Chen and Timothy Chen.
 
 
 Repository: mesos
 
 
 Description
 ---
 
 Added implementation for docker registry's Get Manifest and Get Blob APIs.
 
 
 Diffs
 -
 
   src/Makefile.am 7b620ff66856b3f0adac121b3297d55ed71a3d99 
   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 PRE-CREATION 
 
 Diff: https://reviews.apache.org/r/37773/diff/
 
 
 Testing
 ---
 
 make check
 
 
 Thanks,
 
 Jojy Varghese

Re: Review Request 37773: Docker: Adding registry client.

2015-08-27 Thread Mesos ReviewBot 

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


Bad patch!

Reviews applied: [37426, 37427]

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

Error:
 2015-08-28 01:10:55 URL:https://reviews.apache.org/r/37427/diff/raw/ 
[50896/50896] - 37427.patch [1]
Successfully applied: Docker registry: adding TokenManager.

Changes:
  - Added Token implementation (RFC 7519).
  - Added TokenManager implementation. This component keeps a cache of tokens
  requested for any future requests.


Review: https://reviews.apache.org/r/37427
Checking 7 files using filter 
--filter=-,+build/class,+build/deprecated,+build/endif_comment,+readability/todo,+readability/namespace,+runtime/vlog,+whitespace/blank_line,+whitespace/comma,+whitespace/end_of_line,+whitespace/ending_newline,+whitespace/forcolon,+whitespace/indent,+whitespace/line_length,+whitespace/operators,+whitespace/semicolon,+whitespace/tab,+whitespace/todo
Total errors found: 0
ERROR: Commit spanning multiple projects.

Please use separate commits for mesos, libprocess and stout.

Paths grouped by project:
mesos:
  src/Makefile.am
  src/slave/containerizer/provisioners/docker/token_manager.cpp
  src/slave/containerizer/provisioners/docker/token_manager.hpp
  src/tests/provisioners/docker_provisioner_tests.cpp
libprocess:
  3rdparty/libprocess/include/process/openssl_util.hpp
  3rdparty/libprocess/include/process/tests/ssl_test.hpp
  3rdparty/libprocess/src/openssl_util.cpp
  3rdparty/libprocess/src/openssl_util.hpp
  3rdparty/libprocess/src/tests/ssl_tests.cpp
Failed to commit patch

- Mesos ReviewBot


On Aug. 28, 2015, 12:47 a.m., Jojy Varghese wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviews.apache.org/r/37773/
 ---
 
 (Updated Aug. 28, 2015, 12:47 a.m.)
 
 
 Review request for mesos, Lily Chen and Timothy Chen.
 
 
 Repository: mesos
 
 
 Description
 ---
 
 Added implementation for docker registry's Get Manifest and Get Blob APIs.
 
 
 Diffs
 -
 
   3rdparty/libprocess/include/process/tests/ssl_test.hpp PRE-CREATION 
   src/Makefile.am 7b620ff66856b3f0adac121b3297d55ed71a3d99 
   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 PRE-CREATION 
 
 Diff: https://reviews.apache.org/r/37773/diff/
 
 
 Testing
 ---
 
 make check
 
 
 Thanks,
 
 Jojy Varghese

Re: Review Request 37773: Docker: Adding registry client.

2015-08-27 Thread Jojy Varghese 


 On Aug. 26, 2015, midnight, Lily Chen wrote:
 

This was a WIP for you to get the patch :). Anywaz addressed the issues.


 On Aug. 26, 2015, midnight, Lily Chen wrote:
  src/slave/containerizer/provisioners/docker/registry_client.hpp, line 89
  https://reviews.apache.org/r/37773/diff/1/?file=1052557#file1052557line89
 
  Isn't the authorization server returned as the realm in the 401 
  response? At least for docker hub?

The tokenmanager design had token managers created per realm. So yes currently 
the design remains same. Can change if required.


 On Aug. 26, 2015, midnight, Lily Chen wrote:
  src/slave/containerizer/provisioners/docker/registry_client.hpp, line 157
  https://reviews.apache.org/r/37773/diff/1/?file=1052557#file1052557line157
 
  Fix tupo in regiistry

irony :)


 On Aug. 26, 2015, midnight, Lily Chen wrote:
  src/slave/containerizer/provisioners/docker/registry_client.cpp, line 146
  https://reviews.apache.org/r/37773/diff/1/?file=1052558#file1052558line146
 
  Capitalize Beginning of error messages, same applies throughout.

for internal error messages (that bubbles up), the leaf level messsages are 
recommended to start with lower case. This is so that when the root level 
logger logs the message, it does not look like : Failed to do operation foo: 
Failed to fetch data.


 On Aug. 26, 2015, midnight, Lily Chen wrote:
  src/slave/containerizer/provisioners/docker/registry_client.cpp, line 237
  https://reviews.apache.org/r/37773/diff/1/?file=1052558#file1052558line237
 
  Use explicity type instead of auto and remove space between capture and 
  parameter lists

Saw the pattern in other code. Basically, we are allowed to declare lambdas 
like this. Here the return type is void.


 On Aug. 26, 2015, midnight, Lily Chen wrote:
  src/slave/containerizer/provisioners/docker/registry_client.cpp, line 360
  https://reviews.apache.org/r/37773/diff/1/?file=1052558#file1052558line360
 
  should we make this some sort of constant?

Didnt seem necessary. Is not used anywhere else. URL class uses literal too.


 On Aug. 26, 2015, midnight, Lily Chen wrote:
  src/slave/containerizer/provisioners/docker/registry_client.cpp, line 412
  https://reviews.apache.org/r/37773/diff/1/?file=1052558#file1052558line412
 
  Is casting  to string necessary?

Yes. Compiler cant resolve type 2 times.


 On Aug. 26, 2015, midnight, Lily Chen wrote:
  src/tests/provisioners/docker_provisioner_tests.cpp, line 95
  https://reviews.apache.org/r/37773/diff/1/?file=1052559#file1052559line95
 
  If we remove a bunch of temporary files during teardown, should we 
  consider using a TemporaryDirectoryTest?

Not sure if we need a separate class to do it. Moreover, what we are trying to 
accomplish here is to have the blobs saved in a separate dir and not it the 
tests working directory or sandbox.


- Jojy


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


On Aug. 28, 2015, 12:03 a.m., Jojy Varghese wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviews.apache.org/r/37773/
 ---
 
 (Updated Aug. 28, 2015, 12:03 a.m.)
 
 
 Review request for mesos, Lily Chen and Timothy Chen.
 
 
 Repository: mesos
 
 
 Description
 ---
 
 Added implementation for docker registry's Get Manifest and Get Blob APIs.
 
 
 Diffs
 -
 
   3rdparty/libprocess/include/process/tests/ssl_test.hpp PRE-CREATION 
   src/Makefile.am 571e1ac0f96b2452797a478680b540f2aab63aab 
   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 PRE-CREATION 
 
 Diff: https://reviews.apache.org/r/37773/diff/
 
 
 Testing
 ---
 
 make check
 
 
 Thanks,
 
 Jojy Varghese

Re: Review Request 37773: Docker: Adding registry client.

2015-08-27 Thread Jojy Varghese 

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

(Updated Aug. 28, 2015, 12:47 a.m.)


Review request for mesos, Lily Chen and Timothy Chen.


Changes
---

rebased with master.


Repository: mesos


Description
---

Added implementation for docker registry's Get Manifest and Get Blob APIs.


Diffs (updated)
-

  3rdparty/libprocess/include/process/tests/ssl_test.hpp PRE-CREATION 
  src/Makefile.am 7b620ff66856b3f0adac121b3297d55ed71a3d99 
  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 PRE-CREATION 

Diff: https://reviews.apache.org/r/37773/diff/


Testing
---

make check


Thanks,

Jojy Varghese