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/13 02:49:31 UTC
Review Request 37427: Docker registry: adding TokenManager.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37427/
-----------------------------------------------------------
Review request for mesos, Lily Chen and Timothy Chen.
Repository: mesos
Description
-------
Changes:
- Added Token implementation (RFC 7519).
- Added TokenManager implementation. This component keeps a cache of tokens
requested for any future requests.
Diffs
-----
src/Makefile.am 35ebbbd0bd9c9dd059c02ce3dc22c780b929be81
src/slave/containerizer/provisioners/docker/token_manager.hpp PRE-CREATION
src/slave/containerizer/provisioners/docker/token_manager.cpp PRE-CREATION
src/tests/containerizer/docker_containerizer_tests.cpp 80ed60e2b0fa39e8302867a7cb6a7388c25f9a40
Diff: https://reviews.apache.org/r/37427/diff/
Testing
-------
make check.
Thanks,
Jojy Varghese
Re: Review Request 37427: Docker registry: adding TokenManager.
Posted by Jojy Varghese <jo...@mesosphere.io>.
> On Aug. 13, 2015, 6:24 a.m., Lily Chen wrote:
> > src/slave/containerizer/provisioners/docker/token_manager.cpp, line 252
> > <https://reviews.apache.org/r/37427/diff/2/?file=1039212#file1039212line252>
> >
> > space between () {}, you could also probably just do this in the header file
The reason we dont add definition in header file is that compiler might add the definition in each compilation unit the header file is included. This will bloat the binary (and other issues that comes along with it).
- Jojy
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37427/#review95238
-----------------------------------------------------------
On Aug. 13, 2015, 8:29 a.m., Jojy Varghese wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37427/
> -----------------------------------------------------------
>
> (Updated Aug. 13, 2015, 8:29 a.m.)
>
>
> Review request for mesos, Lily Chen and Timothy Chen.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Changes:
> - Added Token implementation (RFC 7519).
> - Added TokenManager implementation. This component keeps a cache of tokens
> requested for any future requests.
>
>
> Diffs
> -----
>
> src/Makefile.am 111aed92820689b12ee4073269ce34db7be30960
> src/slave/containerizer/provisioners/docker/token_manager.hpp PRE-CREATION
> src/slave/containerizer/provisioners/docker/token_manager.cpp PRE-CREATION
> src/tests/containerizer/docker_containerizer_tests.cpp c8c27a64c06cf37bdaa5b474ea25bd2e971c8fea
>
> Diff: https://reviews.apache.org/r/37427/diff/
>
>
> Testing
> -------
>
> make check.
>
>
> Thanks,
>
> Jojy Varghese
>
>
Re: Review Request 37427: Docker registry: adding TokenManager.
Posted by Jojy Varghese <jo...@mesosphere.io>.
> On Aug. 13, 2015, 6:24 a.m., Lily Chen wrote:
> > src/slave/containerizer/provisioners/docker/token_manager.cpp, lines 37-46
> > <https://reviews.apache.org/r/37427/diff/2/?file=1039212#file1039212line37>
> >
> > no need to put underscore in front of parameters being passed in
As per style docs, its an acceptable way to disambiguate.
> On Aug. 13, 2015, 6:24 a.m., Lily Chen wrote:
> > src/tests/containerizer/docker_containerizer_tests.cpp, line 2998
> > <https://reviews.apache.org/r/37427/diff/2/?file=1039213#file1039213line2998>
> >
> > DockerContainerizerTest tests the Docker Containerizer. The TokenManager is a component of the Mesos Containerizer, and would therefore be a part of a Docker provisioner test file.
I understand. Since other part of proviosioner feature is being worked on in parallel, I have put these tests in this file, as I didnt see a separate file for provisioner tests. I have added a TODO in the test file to move these tests to the new file when it is dropped. Also, these new tests are isolated from the already existing tests in the file.
- Jojy
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37427/#review95238
-----------------------------------------------------------
On Aug. 13, 2015, 8:29 a.m., Jojy Varghese wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37427/
> -----------------------------------------------------------
>
> (Updated Aug. 13, 2015, 8:29 a.m.)
>
>
> Review request for mesos, Lily Chen and Timothy Chen.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Changes:
> - Added Token implementation (RFC 7519).
> - Added TokenManager implementation. This component keeps a cache of tokens
> requested for any future requests.
>
>
> Diffs
> -----
>
> src/Makefile.am 111aed92820689b12ee4073269ce34db7be30960
> src/slave/containerizer/provisioners/docker/token_manager.hpp PRE-CREATION
> src/slave/containerizer/provisioners/docker/token_manager.cpp PRE-CREATION
> src/tests/containerizer/docker_containerizer_tests.cpp c8c27a64c06cf37bdaa5b474ea25bd2e971c8fea
>
> Diff: https://reviews.apache.org/r/37427/diff/
>
>
> Testing
> -------
>
> make check.
>
>
> Thanks,
>
> Jojy Varghese
>
>
Re: Review Request 37427: Docker registry: adding TokenManager.
Posted by Lily Chen <li...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37427/#review95238
-----------------------------------------------------------
src/slave/containerizer/provisioners/docker/token_manager.hpp (line 51)
<https://reviews.apache.org/r/37427/#comment150088>
space between // and TODO
src/slave/containerizer/provisioners/docker/token_manager.hpp (line 53)
<https://reviews.apache.org/r/37427/#comment150087>
Do javadoc style comments begin with "/**" ?
I think the same would apply to other javadoc style comments throughout
src/slave/containerizer/provisioners/docker/token_manager.cpp (lines 37 - 46)
<https://reviews.apache.org/r/37427/#comment150097>
no need to put underscore in front of parameters being passed in
src/slave/containerizer/provisioners/docker/token_manager.cpp (line 135)
<https://reviews.apache.org/r/37427/#comment150095>
space between ) and {
src/slave/containerizer/provisioners/docker/token_manager.cpp (line 230)
<https://reviews.apache.org/r/37427/#comment150093>
pass in token by const reference?
src/slave/containerizer/provisioners/docker/token_manager.cpp (line 252)
<https://reviews.apache.org/r/37427/#comment150094>
space between () {}, you could also probably just do this in the header file
src/tests/containerizer/docker_containerizer_tests.cpp (line 2998)
<https://reviews.apache.org/r/37427/#comment150089>
DockerContainerizerTest tests the Docker Containerizer. The TokenManager is a component of the Mesos Containerizer, and would therefore be a part of a Docker provisioner test file.
- Lily Chen
On Aug. 13, 2015, 4:47 a.m., Jojy Varghese wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37427/
> -----------------------------------------------------------
>
> (Updated Aug. 13, 2015, 4:47 a.m.)
>
>
> Review request for mesos, Lily Chen and Timothy Chen.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Changes:
> - Added Token implementation (RFC 7519).
> - Added TokenManager implementation. This component keeps a cache of tokens
> requested for any future requests.
>
>
> Diffs
> -----
>
> src/Makefile.am 111aed92820689b12ee4073269ce34db7be30960
> src/slave/containerizer/provisioners/docker/token_manager.hpp PRE-CREATION
> src/slave/containerizer/provisioners/docker/token_manager.cpp PRE-CREATION
> src/tests/containerizer/docker_containerizer_tests.cpp c8c27a64c06cf37bdaa5b474ea25bd2e971c8fea
>
> Diff: https://reviews.apache.org/r/37427/diff/
>
>
> Testing
> -------
>
> make check.
>
>
> Thanks,
>
> Jojy Varghese
>
>
Re: Review Request 37427: Docker registry: adding TokenManager.
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37427/#review95243
-----------------------------------------------------------
Patch looks great!
Reviews applied: [37426, 37427]
All tests passed.
- Mesos ReviewBot
On Aug. 13, 2015, 8:29 a.m., Jojy Varghese wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37427/
> -----------------------------------------------------------
>
> (Updated Aug. 13, 2015, 8:29 a.m.)
>
>
> Review request for mesos, Lily Chen and Timothy Chen.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Changes:
> - Added Token implementation (RFC 7519).
> - Added TokenManager implementation. This component keeps a cache of tokens
> requested for any future requests.
>
>
> Diffs
> -----
>
> src/Makefile.am 111aed92820689b12ee4073269ce34db7be30960
> src/slave/containerizer/provisioners/docker/token_manager.hpp PRE-CREATION
> src/slave/containerizer/provisioners/docker/token_manager.cpp PRE-CREATION
> src/tests/containerizer/docker_containerizer_tests.cpp c8c27a64c06cf37bdaa5b474ea25bd2e971c8fea
>
> Diff: https://reviews.apache.org/r/37427/diff/
>
>
> Testing
> -------
>
> make check.
>
>
> Thanks,
>
> Jojy Varghese
>
>
Re: Review Request 37427: Docker registry: adding TokenManager.
Posted by Jojy Varghese <jo...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37427/
-----------------------------------------------------------
(Updated Aug. 13, 2015, 8:29 a.m.)
Review request for mesos, Lily Chen and Timothy Chen.
Changes
-------
build fix; review comments addressed.
Repository: mesos
Description
-------
Changes:
- Added Token implementation (RFC 7519).
- Added TokenManager implementation. This component keeps a cache of tokens
requested for any future requests.
Diffs (updated)
-----
src/Makefile.am 111aed92820689b12ee4073269ce34db7be30960
src/slave/containerizer/provisioners/docker/token_manager.hpp PRE-CREATION
src/slave/containerizer/provisioners/docker/token_manager.cpp PRE-CREATION
src/tests/containerizer/docker_containerizer_tests.cpp c8c27a64c06cf37bdaa5b474ea25bd2e971c8fea
Diff: https://reviews.apache.org/r/37427/diff/
Testing
-------
make check.
Thanks,
Jojy Varghese
Re: Review Request 37427: Docker registry: adding TokenManager.
Posted by Jojy Varghese <jo...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37427/
-----------------------------------------------------------
(Updated Aug. 13, 2015, 4:47 a.m.)
Review request for mesos, Lily Chen and Timothy Chen.
Changes
-------
updating after rebase.
Repository: mesos
Description
-------
Changes:
- Added Token implementation (RFC 7519).
- Added TokenManager implementation. This component keeps a cache of tokens
requested for any future requests.
Diffs (updated)
-----
src/Makefile.am 111aed92820689b12ee4073269ce34db7be30960
src/slave/containerizer/provisioners/docker/token_manager.hpp PRE-CREATION
src/slave/containerizer/provisioners/docker/token_manager.cpp PRE-CREATION
src/tests/containerizer/docker_containerizer_tests.cpp c8c27a64c06cf37bdaa5b474ea25bd2e971c8fea
Diff: https://reviews.apache.org/r/37427/diff/
Testing
-------
make check.
Thanks,
Jojy Varghese
Re: Review Request 37427: Docker registry: adding TokenManager.
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37427/#review95230
-----------------------------------------------------------
Bad patch!
Reviews applied: [37426, 37427]
Failed command: ./support/apply-review.sh -n -r 37427
Error:
2015-08-13 02:59:12 URL:https://reviews.apache.org/r/37427/diff/raw/ [30937/30937] -> "37427.patch" [1]
error: patch failed: src/Makefile.am:529
error: src/Makefile.am: patch does not apply
Failed to apply patch
- Mesos ReviewBot
On Aug. 13, 2015, 12:49 a.m., Jojy Varghese wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37427/
> -----------------------------------------------------------
>
> (Updated Aug. 13, 2015, 12:49 a.m.)
>
>
> Review request for mesos, Lily Chen and Timothy Chen.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Changes:
> - Added Token implementation (RFC 7519).
> - Added TokenManager implementation. This component keeps a cache of tokens
> requested for any future requests.
>
>
> Diffs
> -----
>
> src/Makefile.am 35ebbbd0bd9c9dd059c02ce3dc22c780b929be81
> src/slave/containerizer/provisioners/docker/token_manager.hpp PRE-CREATION
> src/slave/containerizer/provisioners/docker/token_manager.cpp PRE-CREATION
> src/tests/containerizer/docker_containerizer_tests.cpp 80ed60e2b0fa39e8302867a7cb6a7388c25f9a40
>
> Diff: https://reviews.apache.org/r/37427/diff/
>
>
> Testing
> -------
>
> make check.
>
>
> Thanks,
>
> Jojy Varghese
>
>