You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Jojy Varghese <jo...@mesosphere.io> on 2015/08/28 02:03:09 UTC
Re: Review Request 37773: Docker: Adding registry client.
-----------------------------------------------------------
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.
Changes
-------
Changes after refactoring SSL test.
Summary (updated)
-----------------
Docker: Adding registry client.
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 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.
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
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.
Posted by Jojy Varghese <jo...@mesosphere.io>.
> 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.
Posted by Lily Chen <li...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37773/#review96896
-----------------------------------------------------------
src/slave/containerizer/provisioners/docker/registry_client.hpp (line 34)
<https://reviews.apache.org/r/37773/#comment152577>
No need for provisioners namespace, see appc example of mesos::internal::slave::appc
- Lily Chen
On Aug. 28, 2015, 4:26 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, 4:26 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
> -----
>
> 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.
Posted by Jojy Varghese <jo...@mesosphere.io>.
> 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.
Posted by Timothy Chen <tn...@apache.org>.
-----------------------------------------------------------
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.
Posted by Jojy Varghese <jo...@mesosphere.io>.
> On Aug. 31, 2015, 8:42 p.m., Timothy Chen wrote:
> > src/slave/containerizer/provisioners/docker/registry_client.cpp, line 456
> > <https://reviews.apache.org/r/37773/diff/7/?file=1059875#file1059875line456>
> >
> > 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.
Posted by Timothy Chen <tn...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37773/#review97138
-----------------------------------------------------------
src/slave/containerizer/provisioners/docker/registry_client.hpp (line 111)
<https://reviews.apache.org/r/37773/#comment152903>
We are only fetching one blob each call right? so s/fetched blob/fetched blobs/g ?
src/slave/containerizer/provisioners/docker/registry_client.cpp (line 456)
<https://reviews.apache.org/r/37773/#comment152904>
is maxSize being used at all? If not we should remove all references of it.
- Timothy Chen
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.
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37773/#review97029
-----------------------------------------------------------
Patch looks great!
Reviews applied: [37871, 37427, 37773]
All tests passed.
- Mesos ReviewBot
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.
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37773/#review97235
-----------------------------------------------------------
Patch looks great!
Reviews applied: [37871, 37427, 37773]
All tests passed.
- Mesos ReviewBot
On Sept. 1, 2015, 12:02 a.m., Jojy Varghese wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37773/
> -----------------------------------------------------------
>
> (Updated Sept. 1, 2015, 12:02 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
> -----
>
> 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.
Posted by Jojy Varghese <jo...@mesosphere.io>.
> On Sept. 1, 2015, 9:35 p.m., Timothy Chen wrote:
> > src/slave/containerizer/provisioners/docker/registry_client.cpp, line 186
> > <https://reviews.apache.org/r/37773/diff/9/?file=1060828#file1060828line186>
> >
> > 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
> > <https://reviews.apache.org/r/37773/diff/9/?file=1060828#file1060828line214>
> >
> > 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
> > <https://reviews.apache.org/r/37773/diff/9/?file=1060828#file1060828line233>
> >
> > 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
> > <https://reviews.apache.org/r/37773/diff/9/?file=1060828#file1060828line359>
> >
> > 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.
Posted by Jojy Varghese <jo...@mesosphere.io>.
> On Sept. 1, 2015, 9:35 p.m., Timothy Chen wrote:
> > src/tests/provisioners/docker_provisioner_tests.cpp, line 232
> > <https://reviews.apache.org/r/37773/diff/9/?file=1060829#file1060829line232>
> >
> > 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.
Posted by Timothy Chen <tn...@apache.org>.
> On Sept. 1, 2015, 9:35 p.m., Timothy Chen wrote:
> > src/slave/containerizer/provisioners/docker/registry_client.cpp, line 359
> > <https://reviews.apache.org/r/37773/diff/9/?file=1060828#file1060828line359>
> >
> > 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.
Posted by Jojy Varghese <jo...@mesosphere.io>.
> On Sept. 1, 2015, 9:35 p.m., Timothy Chen wrote:
> > src/slave/containerizer/provisioners/docker/registry_client.cpp, line 359
> > <https://reviews.apache.org/r/37773/diff/9/?file=1060828#file1060828line359>
> >
> > 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.
Posted by Jojy Varghese <jo...@mesosphere.io>.
> On Sept. 1, 2015, 9:35 p.m., Timothy Chen wrote:
> > src/slave/containerizer/provisioners/docker/registry_client.cpp, line 410
> > <https://reviews.apache.org/r/37773/diff/9/?file=1060828#file1060828line410>
> >
> > 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
> > <https://reviews.apache.org/r/37773/diff/9/?file=1060828#file1060828line306>
> >
> > 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.
Posted by Jojy Varghese <jo...@mesosphere.io>.
- 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.
Posted by Timothy Chen <tn...@apache.org>.
-----------------------------------------------------------
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)
<https://reviews.apache.org/r/37773/#comment153180>
Remove provisioners namespace for now
src/slave/containerizer/provisioners/docker/registry_client.hpp (line 34)
<https://reviews.apache.org/r/37773/#comment153181>
Remove provisioners namespace for now
src/slave/containerizer/provisioners/docker/registry_client.cpp (line 50)
<https://reviews.apache.org/r/37773/#comment153186>
spell out credentials
src/slave/containerizer/provisioners/docker/registry_client.cpp (line 67)
<https://reviews.apache.org/r/37773/#comment153187>
ditto
src/slave/containerizer/provisioners/docker/registry_client.cpp (line 93)
<https://reviews.apache.org/r/37773/#comment153188>
fix ident (4 spaces, so move 2 spaces left)
src/slave/containerizer/provisioners/docker/registry_client.cpp (line 108)
<https://reviews.apache.org/r/37773/#comment153189>
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)
<https://reviews.apache.org/r/37773/#comment153183>
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)
<https://reviews.apache.org/r/37773/#comment153201>
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)
<https://reviews.apache.org/r/37773/#comment153190>
This should be Option<string> 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)
<https://reviews.apache.org/r/37773/#comment153191>
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)
<https://reviews.apache.org/r/37773/#comment153200>
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)
<https://reviews.apache.org/r/37773/#comment153192>
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)
<https://reviews.apache.org/r/37773/#comment153193>
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)
<https://reviews.apache.org/r/37773/#comment153195>
strings::contains
src/slave/containerizer/provisioners/docker/registry_client.cpp (line 293)
<https://reviews.apache.org/r/37773/#comment153194>
Fix alignment
src/slave/containerizer/provisioners/docker/registry_client.cpp (line 306)
<https://reviews.apache.org/r/37773/#comment153196>
Put this in a constant.
src/slave/containerizer/provisioners/docker/registry_client.cpp (line 309)
<https://reviews.apache.org/r/37773/#comment153197>
Can you comment what's this block is for?
src/slave/containerizer/provisioners/docker/registry_client.cpp (line 315)
<https://reviews.apache.org/r/37773/#comment153198>
Fix indentation.
src/slave/containerizer/provisioners/docker/registry_client.cpp (line 336)
<https://reviews.apache.org/r/37773/#comment153199>
Fix alignment
src/slave/containerizer/provisioners/docker/registry_client.cpp (line 359)
<https://reviews.apache.org/r/37773/#comment153204>
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)
<https://reviews.apache.org/r/37773/#comment153202>
Move this to previous line
src/slave/containerizer/provisioners/docker/registry_client.cpp (line 401)
<https://reviews.apache.org/r/37773/#comment153203>
don't take reference for size_t, just pass by value
src/slave/containerizer/provisioners/docker/registry_client.cpp (line 410)
<https://reviews.apache.org/r/37773/#comment153208>
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.
src/tests/provisioners/docker_provisioner_tests.cpp (line 34)
<https://reviews.apache.org/r/37773/#comment153209>
This goes before token_manager
src/tests/provisioners/docker_provisioner_tests.cpp (line 231)
<https://reviews.apache.org/r/37773/#comment153213>
extra space between if and (
src/tests/provisioners/docker_provisioner_tests.cpp (line 232)
<https://reviews.apache.org/r/37773/#comment153214>
Why is this needed?
- Timothy Chen
On Sept. 1, 2015, 12:02 a.m., Jojy Varghese wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37773/
> -----------------------------------------------------------
>
> (Updated Sept. 1, 2015, 12:02 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
> -----
>
> 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.
Posted by Jojy Varghese <jo...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37773/
-----------------------------------------------------------
(Updated Sept. 1, 2015, 12:02 a.m.)
Review request for mesos, Lily Chen and Timothy Chen.
Changes
-------
got rid of promise construct.
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.
Posted by Jojy Varghese <jo...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37773/
-----------------------------------------------------------
(Updated Aug. 31, 2015, 10:40 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 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.
Posted by Jojy Varghese <jo...@mesosphere.io>.
-----------------------------------------------------------
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.
Changes
-------
review comments
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.
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37773/#review96908
-----------------------------------------------------------
Patch looks great!
Reviews applied: [37871, 37427, 37773]
All tests passed.
- Mesos ReviewBot
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.
Posted by Jojy Varghese <jo...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37773/#review96957
-----------------------------------------------------------
src/slave/containerizer/provisioners/docker/registry_client.cpp (lines 468 - 469)
<https://reviews.apache.org/r/37773/#comment152658>
The default has already been taken care of in the caller.
- Jojy Varghese
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.
Posted by Jojy Varghese <jo...@mesosphere.io>.
> On Aug. 28, 2015, 11:03 p.m., Lily Chen wrote:
> > src/slave/containerizer/provisioners/docker/registry_client.cpp, line 465
> > <https://reviews.apache.org/r/37773/diff/6/?file=1058283#file1058283line465>
> >
> > Same issue with path naming as with getManifest
Please see https://docs.docker.com/registry/spec/api/. 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.
Posted by Jojy Varghese <jo...@mesosphere.io>.
> 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.
Posted by Jojy Varghese <jo...@mesosphere.io>.
> 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.
Posted by Lily Chen <li...@mesosphere.io>.
-----------------------------------------------------------
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.
Posted by Jojy Varghese <jo...@mesosphere.io>.
> On Aug. 28, 2015, 6:48 p.m., Timothy Chen wrote:
> > I have a feeling we can remove all the promises usage in this review. Let's chat offline.
Conceptually an async operation caller creates a promise and returns that promise's future. It then does the async operation which sets the future's value using the promise that was created. This is what is captured in the code.
- Jojy
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37773/#review96907
-----------------------------------------------------------
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.
Posted by Timothy Chen <tn...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37773/#review96907
-----------------------------------------------------------
I have a feeling we can remove all the promises usage in this review. Let's chat offline.
- 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.
Posted by Jojy Varghese <jo...@mesosphere.io>.
-----------------------------------------------------------
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.
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37773/#review96830
-----------------------------------------------------------
Bad patch!
Reviews applied: [37871, 37427, 37773]
Failed command: make -j3 distcheck
Error:
make dist-gzip am__post_remove_distdir='@:'
make[1]: Entering directory `/home/jenkins/jenkins-slave/workspace/mesos-reviewbot'
if test -d "mesos-0.25.0"; then find "mesos-0.25.0" -type d ! -perm -200 -exec chmod u+w {} ';' && rm -rf "mesos-0.25.0" || { sleep 5 && rm -rf "mesos-0.25.0"; }; else :; fi
test -d "mesos-0.25.0" || mkdir "mesos-0.25.0"
(cd 3rdparty && make top_distdir=../mesos-0.25.0 distdir=../mesos-0.25.0/3rdparty \
am__remove_distdir=: am__skip_length_check=: am__skip_mode_fix=: distdir)
make[2]: Entering directory `/home/jenkins/jenkins-slave/workspace/mesos-reviewbot/3rdparty'
(cd libprocess && make top_distdir=../../mesos-0.25.0 distdir=../../mesos-0.25.0/3rdparty/libprocess \
am__remove_distdir=: am__skip_length_check=: am__skip_mode_fix=: distdir)
make[3]: Entering directory `/home/jenkins/jenkins-slave/workspace/mesos-reviewbot/3rdparty/libprocess'
make[3]: *** No rule to make target `src/openssl_util.hpp', needed by `distdir'. Stop.
make[3]: Leaving directory `/home/jenkins/jenkins-slave/workspace/mesos-reviewbot/3rdparty/libprocess'
make[2]: *** [distdir] Error 1
make[2]: Leaving directory `/home/jenkins/jenkins-slave/workspace/mesos-reviewbot/3rdparty'
make[1]: *** [distdir] Error 1
make[1]: Leaving directory `/home/jenkins/jenkins-slave/workspace/mesos-reviewbot'
make: *** [dist] Error 2
- Mesos ReviewBot
On Aug. 28, 2015, 4:26 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, 4:26 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
> -----
>
> 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.
Posted by Jojy Varghese <jo...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37773/
-----------------------------------------------------------
(Updated Aug. 28, 2015, 4:26 a.m.)
Review request for mesos, Lily Chen and Timothy Chen.
Changes
-------
after splitting ssl test patch.
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.
Posted by Jojy Varghese <jo...@mesosphere.io>.
-----------------------------------------------------------
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