Re: Review Request 37427: Docker registry: adding TokenManager.

[Jojy Varghese]

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

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

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 0a8ef6d8551cf177cb565b2a443c05e8eea5ab1c 
  src/slave/containerizer/provisioners/docker/token_manager.hpp PRE-CREATION 
  src/slave/containerizer/provisioners/docker/token_manager.cpp PRE-CREATION 
  src/tests/provisioners/docker_provisioner_tests.cpp PRE-CREATION 

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


Testing
---

make check.


Thanks,

Jojy Varghese

Re: Review Request 37427: Docker registry: adding TokenManager.

[Timothy Chen]

> On Sept. 2, 2015, 8:01 p.m., Anand Mazumdar wrote:
> > src/Makefile.am, line 761
> > 
> >
> > Extra space ?
> 
> Jojy Varghese wrote:
> looks to be aligned with the rest
> 
> Anand Mazumdar wrote:
> The '\''s at the end does not seem to be aligned.

I'll fx it


- Timothy


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


On Sept. 2, 2015, 10:50 p.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37427/
> ---
> 
> (Updated Sept. 2, 2015, 10:50 p.m.)
> 
> 
> Review request for mesos, Lily Chen, Joris Van Remoortere, 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 7b4d9f65506e7fa8425966009401aae73cdb79a5 
>   src/slave/containerizer/provisioners/docker/token_manager.hpp PRE-CREATION 
>   src/slave/containerizer/provisioners/docker/token_manager.cpp PRE-CREATION 
>   src/tests/provisioners/docker_provisioner_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/37427/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>

Re: Review Request 37427: Docker registry: adding TokenManager.

[Jojy Varghese]

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

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


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


Changes
---

review comments.


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 7b4d9f65506e7fa8425966009401aae73cdb79a5 
  src/slave/containerizer/provisioners/docker/token_manager.hpp PRE-CREATION 
  src/slave/containerizer/provisioners/docker/token_manager.cpp PRE-CREATION 
  src/tests/provisioners/docker_provisioner_tests.cpp PRE-CREATION 

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


Testing
---

make check.


Thanks,

Jojy Varghese

Re: Review Request 37427: Docker registry: adding TokenManager.

[Anand Mazumdar]

> On Sept. 2, 2015, 8:01 p.m., Anand Mazumdar wrote:
> > src/slave/containerizer/provisioners/docker/token_manager.hpp, line 19
> > 
> >
> > Can we change this to be __MESOS_DOCKER_TOKEN_MANAGER_HPP__ 
> > 
> > to be in sync with 
> > https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#The__define_Guard
> > 
> > The one's in appc/* seem to be using __MESOS_APPC_STORE__ which seems 
> > to be correct.
> 
> Jojy Varghese wrote:
> Most of the source code in mesos follow this style (my reference was 
> http.hpp).
> 
> Anand Mazumdar wrote:
> Which http.hpp are we talking about here ? If you are referring to 
> include/process/http.hpp , it already does the right thing i.e. 
> PROCESS_HTTP_HPP ?
> 
> Jojy Varghese wrote:
> I must be misunderstanding what you are trying to say. google style guide 
> asks to have the header guard to follow directory path. Here the dierctory 
> path is too deep. So the relative path is slave/containerizer. Then the rest 
> of the path is provisioner/docker. We dont follow that in any provisioner 
> headers now. The uniqueness about this header is that it is a "provisioner" 
> of type "docker" and belongs to the "registry" namespace. I have the extra 
> "registry" in the guard to reflect that.

I was simply referring to us not having "namespace" names in the header guard 
includes and just using "shorter" relative paths as is being done in other 
parts of the code. Hence, I asked you to remove the "registry" in the guard.

Had no intentions of dragging on this trivial thing !


- Anand


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


On Sept. 2, 2015, 5:07 p.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37427/
> ---
> 
> (Updated Sept. 2, 2015, 5:07 p.m.)
> 
> 
> Review request for mesos, Lily Chen, Joris Van Remoortere, 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 7b4d9f65506e7fa8425966009401aae73cdb79a5 
>   src/slave/containerizer/provisioners/docker/token_manager.hpp PRE-CREATION 
>   src/slave/containerizer/provisioners/docker/token_manager.cpp PRE-CREATION 
>   src/tests/provisioners/docker_provisioner_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/37427/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>

Re: Review Request 37427: Docker registry: adding TokenManager.

[Jojy Varghese]

> On Sept. 2, 2015, 8:01 p.m., Anand Mazumdar wrote:
> > src/slave/containerizer/provisioners/docker/token_manager.hpp, line 19
> > 
> >
> > Can we change this to be __MESOS_DOCKER_TOKEN_MANAGER_HPP__ 
> > 
> > to be in sync with 
> > https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#The__define_Guard
> > 
> > The one's in appc/* seem to be using __MESOS_APPC_STORE__ which seems 
> > to be correct.
> 
> Jojy Varghese wrote:
> Most of the source code in mesos follow this style (my reference was 
> http.hpp).
> 
> Anand Mazumdar wrote:
> Which http.hpp are we talking about here ? If you are referring to 
> include/process/http.hpp , it already does the right thing i.e. 
> PROCESS_HTTP_HPP ?

I must be misunderstanding what you are trying to say. google style guide asks 
to have the header guard to follow directory path. Here the dierctory path is 
too deep. So the relative path is slave/containerizer. Then the rest of the 
path is provisioner/docker. We dont follow that in any provisioner headers now. 
The uniqueness about this header is that it is a "provisioner" of type "docker" 
and belongs to the "registry" namespace. I have the extra "registry" in the 
guard to reflect that.


> On Sept. 2, 2015, 8:01 p.m., Anand Mazumdar wrote:
> > src/slave/containerizer/provisioners/docker/token_manager.cpp, line 300
> > 
> >
> > Nit: Why not just key ?
> 
> Jojy Varghese wrote:
> to be explicit.
> 
> Anand Mazumdar wrote:
> Looks pretty redundant here. "key" can refer to only one thing inside 
> this function and that being a "tokenKey"

:). Its a personal choice.


> On Sept. 2, 2015, 8:01 p.m., Anand Mazumdar wrote:
> > src/slave/containerizer/provisioners/docker/token_manager.cpp, line 24
> > 
> >
> > This is usually frowned upon. Whatever, occurences we have left of this 
> > in our code-base, we should strive towards removing them. Can you 
> > selectively include what you need ?
> 
> Jojy Varghese wrote:
> hrm.. i need process namespace.
> 
> Anand Mazumdar wrote:
> Why do you need the entire namespace here ? You should be able to 
> selectively use whatever you want from the 'process' namespace or am I 
> missing something ?

I understand namespace pollution issue but its a common pattern i saw in most 
of the cpp files.


- Jojy


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


On Sept. 2, 2015, 5:07 p.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37427/
> ---
> 
> (Updated Sept. 2, 2015, 5:07 p.m.)
> 
> 
> Review request for mesos, Lily Chen, Joris Van Remoortere, 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 7b4d9f65506e7fa8425966009401aae73cdb79a5 
>   src/slave/containerizer/provisioners/docker/token_manager.hpp PRE-CREATION 
>   src/slave/containerizer/provisioners/docker/token_manager.cpp PRE-CREATION 
>   src/tests/provisioners/docker_provisioner_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/37427/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>

Re: Review Request 37427: Docker registry: adding TokenManager.

[Anand Mazumdar]

> On Sept. 2, 2015, 8:01 p.m., Anand Mazumdar wrote:
> > src/slave/containerizer/provisioners/docker/token_manager.hpp, line 19
> > 
> >
> > Can we change this to be __MESOS_DOCKER_TOKEN_MANAGER_HPP__ 
> > 
> > to be in sync with 
> > https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#The__define_Guard
> > 
> > The one's in appc/* seem to be using __MESOS_APPC_STORE__ which seems 
> > to be correct.
> 
> Jojy Varghese wrote:
> Most of the source code in mesos follow this style (my reference was 
> http.hpp).

Which http.hpp are we talking about here ? If you are referring to 
include/process/http.hpp , it already does the right thing i.e. 
PROCESS_HTTP_HPP ?


> On Sept. 2, 2015, 8:01 p.m., Anand Mazumdar wrote:
> > src/Makefile.am, line 761
> > 
> >
> > Extra space ?
> 
> Jojy Varghese wrote:
> looks to be aligned with the rest

The '\''s at the end does not seem to be aligned.


> On Sept. 2, 2015, 8:01 p.m., Anand Mazumdar wrote:
> > src/slave/containerizer/provisioners/docker/token_manager.hpp, line 79
> > 
> >
> > Remove this TODO ? You already have it in the cpp file ?
> 
> Jojy Varghese wrote:
> I doubt that cpp file has the same TODO.

My bad, the other's were for validation. Dropped.


> On Sept. 2, 2015, 8:01 p.m., Anand Mazumdar wrote:
> > src/slave/containerizer/provisioners/docker/token_manager.hpp, line 82
> > 
> >
> > Token(
> > const std::string& raw,
> > const JSON::Object& headerJson,
> > const JSON::Object& claimsJson,
> > const Option& expireTime,
> > const Option& notBeforeTime);
> 
> Jojy Varghese wrote:
> I believe the pattern in code is an acceptable one according to the 
> "Function Definition/Invocation" of the style guide.

Then follow one style and not mix and match. There are other occurences in 
review that seem to follow this pattern. I wonder what's the harm in sticking 
to one ?


> On Sept. 2, 2015, 8:01 p.m., Anand Mazumdar wrote:
> > src/slave/containerizer/provisioners/docker/token_manager.hpp, line 116
> > 
> >
> > Should we indent all @param across multi-lines similar to what we do 
> > for @returns ?
> 
> Jojy Varghese wrote:
> No. If you look at the doxygen guide for mesos, params are separated by 
> single space (not aligned)

sgtm.


> On Sept. 2, 2015, 8:01 p.m., Anand Mazumdar wrote:
> > src/slave/containerizer/provisioners/docker/token_manager.hpp, line 104
> > 
> >
> > Space after TODO
> 
> Jojy Varghese wrote:
> TODO(jojy): is the correct patter, unless i understand your comment wrong.

Dropped.


> On Sept. 2, 2015, 8:01 p.m., Anand Mazumdar wrote:
> > src/slave/containerizer/provisioners/docker/token_manager.cpp, line 24
> > 
> >
> > This is usually frowned upon. Whatever, occurences we have left of this 
> > in our code-base, we should strive towards removing them. Can you 
> > selectively include what you need ?
> 
> Jojy Varghese wrote:
> hrm.. i need process namespace.

Why do you need the entire namespace here ? You should be able to selectively 
use whatever you want from the 'process' namespace or am I missing something ?


> On Sept. 2, 2015, 8:01 p.m., Anand Mazumdar wrote:
> > src/slave/containerizer/provisioners/docker/token_manager.cpp, line 167
> > 
> >
> > Time notBefore; ?
> 
> Jojy Varghese wrote:
> No i meant option.

Ahh, I see. Dropped.


> On Sept. 2, 2015, 8:01 p.m., Anand Mazumdar wrote:
> > src/slave/containerizer/provisioners/docker/token_manager.cpp, line 300
> > 
> >
> > Nit: Why not just key ?
> 
> Jojy Varghese wrote:
> to be explicit.

Looks pretty redundant here. "key" can refer to only one thing inside this 
function and that being a "tokenKey"


- Anand


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


On Sept. 2, 2015, 5:07 p.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37427/
> ---
> 
> (Updated Sept. 2, 2015, 5:07 p.m.)
> 
> 
> Review re

Re: Review Request 37427: Docker registry: adding TokenManager.

[Jojy Varghese]

> On Sept. 2, 2015, 8:01 p.m., Anand Mazumdar wrote:
> > Nothing major, Just some fly-by style comments. Looking at the 
> > implementation in greater detail now.
> > 
> > I wonder if we should split this review into smaller chunks. It currently 
> > stands at ~900 lines. It's very hard to review this otherwise. A possible 
> > trivial split can be to separate the implementation/tests. Thoughts ?

The review is already a sub-unit of the larger registry client patch.


> On Sept. 2, 2015, 8:01 p.m., Anand Mazumdar wrote:
> > src/Makefile.am, line 484
> > 
> >
> > What's the rationale behind moving these ? They had a pattern of the 
> > .cpp/.hpp files being together and now only *some* of them follow this 
> > style.

I thought an early reviewer asked me to do it. I might have misunderstood.


> On Sept. 2, 2015, 8:01 p.m., Anand Mazumdar wrote:
> > src/Makefile.am, line 761
> > 
> >
> > Extra space ?

looks to be aligned with the rest


> On Sept. 2, 2015, 8:01 p.m., Anand Mazumdar wrote:
> > src/slave/containerizer/provisioners/docker/token_manager.hpp, line 19
> > 
> >
> > Can we change this to be __MESOS_DOCKER_TOKEN_MANAGER_HPP__ 
> > 
> > to be in sync with 
> > https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#The__define_Guard
> > 
> > The one's in appc/* seem to be using __MESOS_APPC_STORE__ which seems 
> > to be correct.

Most of the source code in mesos follow this style (my reference was http.hpp).


> On Sept. 2, 2015, 8:01 p.m., Anand Mazumdar wrote:
> > src/slave/containerizer/provisioners/docker/token_manager.hpp, line 79
> > 
> >
> > Remove this TODO ? You already have it in the cpp file ?

I doubt that cpp file has the same TODO.


> On Sept. 2, 2015, 8:01 p.m., Anand Mazumdar wrote:
> > src/slave/containerizer/provisioners/docker/token_manager.hpp, line 82
> > 
> >
> > Token(
> > const std::string& raw,
> > const JSON::Object& headerJson,
> > const JSON::Object& claimsJson,
> > const Option& expireTime,
> > const Option& notBeforeTime);

I believe the pattern in code is an acceptable one according to the "Function 
Definition/Invocation" of the style guide.


> On Sept. 2, 2015, 8:01 p.m., Anand Mazumdar wrote:
> > src/slave/containerizer/provisioners/docker/token_manager.hpp, line 88
> > 
> >
> > static Result getTimeValue(
> > const JSON::Object& object,
> > const std::string& key);

same as above.


> On Sept. 2, 2015, 8:01 p.m., Anand Mazumdar wrote:
> > src/slave/containerizer/provisioners/docker/token_manager.hpp, line 104
> > 
> >
> > Space after TODO

TODO(jojy): is the correct patter, unless i understand your comment wrong.


> On Sept. 2, 2015, 8:01 p.m., Anand Mazumdar wrote:
> > src/slave/containerizer/provisioners/docker/token_manager.hpp, line 116
> > 
> >
> > Should we indent all @param across multi-lines similar to what we do 
> > for @returns ?

No. If you look at the doxygen guide for mesos, params are separated by single 
space (not aligned)


> On Sept. 2, 2015, 8:01 p.m., Anand Mazumdar wrote:
> > src/slave/containerizer/provisioners/docker/token_manager.cpp, line 24
> > 
> >
> > This is usually frowned upon. Whatever, occurences we have left of this 
> > in our code-base, we should strive towards removing them. Can you 
> > selectively include what you need ?

hrm.. i need process namespace.


> On Sept. 2, 2015, 8:01 p.m., Anand Mazumdar wrote:
> > src/slave/containerizer/provisioners/docker/token_manager.cpp, line 25
> > 
> >
> > Same as above ?

i need process::http namespace.


> On Sept. 2, 2015, 8:01 p.m., Anand Mazumdar wrote:
> > src/slave/containerizer/provisioners/docker/token_manager.cpp, line 167
> > 
> >
> > Time notBefore; ?

No i meant option.


> On Sept. 2, 2015, 8:01 p.m., Anand Mazumdar wrote:
> > src/slave/containerizer/provisioners/docker/token_manager.cpp, line 205
> > 
> >
> > How about just :
> > 
> > return expiration.isSome() && Clock::now() >= expiration.get() ?

for readability.


> On Sept. 2, 2

Re: Review Request 37427: Docker registry: adding TokenManager.

[Anand Mazumdar]

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


Nothing major, Just some fly-by style comments. Looking at the implementation 
in greater detail now.

I wonder if we should split this review into smaller chunks. It currently 
stands at ~900 lines. It's very hard to review this otherwise. A possible 
trivial split can be to separate the implementation/tests. Thoughts ?


src/Makefile.am 


What's the rationale behind moving these ? They had a pattern of the 
.cpp/.hpp files being together and now only *some* of them follow this style.



src/Makefile.am (line 756)


Extra space ?



src/Makefile.am (line 758)


Ditto. Same for the other 3 occurences



src/slave/containerizer/provisioners/docker/token_manager.hpp (line 19)


Can we change this to be __MESOS_DOCKER_TOKEN_MANAGER_HPP__ 

to be in sync with 
https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#The__define_Guard

The one's in appc/* seem to be using __MESOS_APPC_STORE__ which seems to be 
correct.



src/slave/containerizer/provisioners/docker/token_manager.hpp (line 56)


Nit: Should this be named just rawToken or just raw as is the case with the 
other occurences ?



src/slave/containerizer/provisioners/docker/token_manager.hpp (line 76)






src/slave/containerizer/provisioners/docker/token_manager.hpp (line 79)


Remove this TODO ? You already have it in the cpp file ?



src/slave/containerizer/provisioners/docker/token_manager.hpp (line 82)


Token(
const std::string& raw,
const JSON::Object& headerJson,
const JSON::Object& claimsJson,
const Option& expireTime,
const Option& notBeforeTime);



src/slave/containerizer/provisioners/docker/token_manager.hpp (line 88)


static Result getTimeValue(
const JSON::Object& object,
const std::string& key);



src/slave/containerizer/provisioners/docker/token_manager.hpp (line 104)


Space after TODO



src/slave/containerizer/provisioners/docker/token_manager.hpp (line 116)


Should we indent all @param across multi-lines similar to what we do for 
@returns ?



src/slave/containerizer/provisioners/docker/token_manager.cpp (line 24)


This is usually frowned upon. Whatever, occurences we have left of this in 
our code-base, we should strive towards removing them. Can you selectively 
include what you need ?



src/slave/containerizer/provisioners/docker/token_manager.cpp (line 25)


Same as above ?



src/slave/containerizer/provisioners/docker/token_manager.cpp (line 48)


Nit: s/std::string/string . There might be more occurences too.



src/slave/containerizer/provisioners/docker/token_manager.cpp (line 120)


Nit: Do you even need this ?

Why not just do segment.length() % 4 on the next line ?



src/slave/containerizer/provisioners/docker/token_manager.cpp (line 167)


Time notBefore; ?



src/slave/containerizer/provisioners/docker/token_manager.cpp (line 175)


Indent



src/slave/containerizer/provisioners/docker/token_manager.cpp (line 205)


How about just :

return expiration.isSome() && Clock::now() >= expiration.get() ?



src/slave/containerizer/provisioners/docker/token_manager.cpp (line 278)


Nit: s/tokenString/token ?



src/slave/containerizer/provisioners/docker/token_manager.cpp (line 286)


Nit: Why the blank-line? We only insert an assingment > 80 chars on the 
previous line



src/slave/containerizer/provisioners/docker/token_manager.cpp (line 300)


Nit: Why not just key ?



src/slave/containerizer/provisioners/docker/token_manager.cpp (line 308)


Remove the "." . We don't use periods to end a LOG line.

Re: Review Request 37427: Docker registry: adding TokenManager.

[Jojy Varghese]

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

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


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


Changes
---

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 7b4d9f65506e7fa8425966009401aae73cdb79a5 
  src/slave/containerizer/provisioners/docker/token_manager.hpp PRE-CREATION 
  src/slave/containerizer/provisioners/docker/token_manager.cpp PRE-CREATION 
  src/tests/provisioners/docker_provisioner_tests.cpp PRE-CREATION 

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


Testing
---

make check.


Thanks,

Jojy Varghese

Re: Review Request 37427: Docker registry: adding TokenManager.

[Jojy Varghese]

> On Sept. 1, 2015, 9:13 p.m., Jojy Varghese wrote:
> > src/slave/containerizer/provisioners/docker/token_manager.cpp, line 61
> > 
> >
> > Its effectively the same. const reference as method argument is just a 
> > good practice, thats all.
> 
> Anand Mazumdar wrote:
> Can you elaborate a bit more on what does the const reference buys us 
> here since you end up removing the const-ness later ? 
> 
> Anyways if you still want to keep it why not name it as a continuation 
> variable ? Something similar to this:
> 
> string segment_(segment); // Remove const.

We dont remove the const'ness of the original string. The original string 
object is unchanged. So now we have to decide if we want to copy the object 
when calling the function or you want to copy the object inside the function. 
The choice made here is to keep the style of "passing by const reference" and 
do teh copy inside the function.


- Jojy


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


On Aug. 31, 2015, 10:39 p.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37427/
> ---
> 
> (Updated Aug. 31, 2015, 10:39 p.m.)
> 
> 
> Review request for mesos, Lily Chen, Joris Van Remoortere, 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 7b4d9f65506e7fa8425966009401aae73cdb79a5 
>   src/slave/containerizer/provisioners/docker/token_manager.hpp PRE-CREATION 
>   src/slave/containerizer/provisioners/docker/token_manager.cpp PRE-CREATION 
>   src/tests/provisioners/docker_provisioner_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/37427/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>

Re: Review Request 37427: Docker registry: adding TokenManager.

[Jojy Varghese]

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



src/tests/provisioners/docker_provisioner_tests.cpp (line 341)


URL is cooked (not real).


- Jojy Varghese


On Aug. 31, 2015, 10:39 p.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37427/
> ---
> 
> (Updated Aug. 31, 2015, 10:39 p.m.)
> 
> 
> Review request for mesos, Lily Chen, Joris Van Remoortere, 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 7b4d9f65506e7fa8425966009401aae73cdb79a5 
>   src/slave/containerizer/provisioners/docker/token_manager.hpp PRE-CREATION 
>   src/slave/containerizer/provisioners/docker/token_manager.cpp PRE-CREATION 
>   src/tests/provisioners/docker_provisioner_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/37427/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>

Re: Review Request 37427: Docker registry: adding TokenManager.

[Anand Mazumdar]

> On Sept. 1, 2015, 9:13 p.m., Jojy Varghese wrote:
> > src/slave/containerizer/provisioners/docker/token_manager.cpp, line 61
> > 
> >
> > Its effectively the same. const reference as method argument is just a 
> > good practice, thats all.

Can you elaborate a bit more on what does the const reference buys us here 
since you end up removing the const-ness later ? 

Anyways if you still want to keep it why not name it as a continuation variable 
? Something similar to this:

string segment_(segment); // Remove const.


- Anand


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


On Aug. 31, 2015, 10:39 p.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37427/
> ---
> 
> (Updated Aug. 31, 2015, 10:39 p.m.)
> 
> 
> Review request for mesos, Lily Chen, Joris Van Remoortere, 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 7b4d9f65506e7fa8425966009401aae73cdb79a5 
>   src/slave/containerizer/provisioners/docker/token_manager.hpp PRE-CREATION 
>   src/slave/containerizer/provisioners/docker/token_manager.cpp PRE-CREATION 
>   src/tests/provisioners/docker_provisioner_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/37427/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>

Re: Review Request 37427: Docker registry: adding TokenManager.

[Jojy Varghese]

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



src/slave/containerizer/provisioners/docker/token_manager.cpp (line 61)


Its effectively the same. const reference as method argument is just a good 
practice, thats all.


- Jojy Varghese


On Aug. 31, 2015, 10:39 p.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37427/
> ---
> 
> (Updated Aug. 31, 2015, 10:39 p.m.)
> 
> 
> Review request for mesos, Lily Chen, Joris Van Remoortere, 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 7b4d9f65506e7fa8425966009401aae73cdb79a5 
>   src/slave/containerizer/provisioners/docker/token_manager.hpp PRE-CREATION 
>   src/slave/containerizer/provisioners/docker/token_manager.cpp PRE-CREATION 
>   src/tests/provisioners/docker_provisioner_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/37427/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>

Re: Review Request 37427: Docker registry: adding TokenManager.

[Timothy Chen]

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

Ship it!


Ship It!


src/slave/containerizer/provisioners/docker/token_manager.hpp (line 38)


Let's remove the provisioners namespace to be at the same level with appc 
provisioner



src/slave/containerizer/provisioners/docker/token_manager.hpp (line 207)


You should move the Process into the cpp file if you don't plan to inherit 
this for mocking.



src/slave/containerizer/provisioners/docker/token_manager.cpp (line 61)


Why not just take a copied value, so
just [](string segment)?



src/slave/containerizer/provisioners/docker/token_manager.cpp (line 299)


Align this next to ( :

return Failure("Failed to parse JSON Web .." + 
   token.error());



src/tests/provisioners/docker_provisioner_tests.cpp (line 341)


I can't tell what's invalid about this?


- Timothy Chen


On Aug. 31, 2015, 10:39 p.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37427/
> ---
> 
> (Updated Aug. 31, 2015, 10:39 p.m.)
> 
> 
> Review request for mesos, Lily Chen, Joris Van Remoortere, 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 7b4d9f65506e7fa8425966009401aae73cdb79a5 
>   src/slave/containerizer/provisioners/docker/token_manager.hpp PRE-CREATION 
>   src/slave/containerizer/provisioners/docker/token_manager.cpp PRE-CREATION 
>   src/tests/provisioners/docker_provisioner_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/37427/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>

Re: Review Request 37427: Docker registry: adding TokenManager.

[Jojy Varghese]

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

(Updated Aug. 31, 2015, 10:39 p.m.)


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


Changes
---

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 7b4d9f65506e7fa8425966009401aae73cdb79a5 
  src/slave/containerizer/provisioners/docker/token_manager.hpp PRE-CREATION 
  src/slave/containerizer/provisioners/docker/token_manager.cpp PRE-CREATION 
  src/tests/provisioners/docker_provisioner_tests.cpp PRE-CREATION 

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


Testing
---

make check.


Thanks,

Jojy Varghese

Re: Review Request 37427: Docker registry: adding TokenManager.

[Jojy Varghese]

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

(Updated Aug. 30, 2015, 3:10 p.m.)


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


Changes
---

review comments.


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 7b620ff66856b3f0adac121b3297d55ed71a3d99 
  src/slave/containerizer/provisioners/docker/token_manager.hpp PRE-CREATION 
  src/slave/containerizer/provisioners/docker/token_manager.cpp PRE-CREATION 
  src/tests/provisioners/docker_provisioner_tests.cpp PRE-CREATION 

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


Testing
---

make check.


Thanks,

Jojy Varghese

Re: Review Request 37427: Docker registry: adding TokenManager.

[Jojy Varghese]

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

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


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


Changes
---

review comments.


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 7b620ff66856b3f0adac121b3297d55ed71a3d99 
  src/slave/containerizer/provisioners/docker/token_manager.hpp PRE-CREATION 
  src/slave/containerizer/provisioners/docker/token_manager.cpp PRE-CREATION 
  src/tests/provisioners/docker_provisioner_tests.cpp PRE-CREATION 

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


Testing
---

make check.


Thanks,

Jojy Varghese

Re: Review Request 37427: Docker registry: adding TokenManager.

[Jojy Varghese]

> On Aug. 28, 2015, 12:18 a.m., Timothy Chen wrote:
> > src/slave/containerizer/provisioners/docker/token_manager.hpp, line 223
> > 
> >
> > Is there a reason you need to include this in the header? Can we just 
> > put it in cpp?
> 
> Jojy Varghese wrote:
> The only reason being that this is a property of the TokenManager.
> 
> Timothy Chen wrote:
> I see, does it need to be? Can't we define a static const in the cpp?

We could but then it wont be a class property. Here the constant reflects the 
property of the class.


- Jojy


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


On Aug. 28, 2015, 4:27 a.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37427/
> ---
> 
> (Updated Aug. 28, 2015, 4:27 a.m.)
> 
> 
> Review request for mesos, Lily Chen, Joris Van Remoortere, 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 7b620ff66856b3f0adac121b3297d55ed71a3d99 
>   src/slave/containerizer/provisioners/docker/token_manager.hpp PRE-CREATION 
>   src/slave/containerizer/provisioners/docker/token_manager.cpp PRE-CREATION 
>   src/tests/provisioners/docker_provisioner_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/37427/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>

Re: Review Request 37427: Docker registry: adding TokenManager.

[Jojy Varghese]

> On Aug. 28, 2015, 7:56 a.m., Timothy Chen wrote:
> > src/slave/containerizer/provisioners/docker/token_manager.cpp, line 223
> > 
> >
> > If you put the cache here in TokenManager instead of 
> > TokenManagerProcess then you'll going to be concurrent access of all the 
> > fields since it's not serialized by libprocess.
> > 
> > I think tokenCache_ will have undefined behavior  when you run 
> > at/contains while it's being inserted  or deleted then.
> > 
> > I suggest you move all this into the process instead.

Here the getToken is called in the context of a "dispatch" from RegistryClient. 
Do we still need another dispatch?


- Jojy


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


On Aug. 28, 2015, 4:27 a.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37427/
> ---
> 
> (Updated Aug. 28, 2015, 4:27 a.m.)
> 
> 
> Review request for mesos, Lily Chen, Joris Van Remoortere, 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 7b620ff66856b3f0adac121b3297d55ed71a3d99 
>   src/slave/containerizer/provisioners/docker/token_manager.hpp PRE-CREATION 
>   src/slave/containerizer/provisioners/docker/token_manager.cpp PRE-CREATION 
>   src/tests/provisioners/docker_provisioner_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/37427/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>

Re: Review Request 37427: Docker registry: adding TokenManager.

[Timothy Chen]

> On Aug. 28, 2015, 12:18 a.m., Timothy Chen wrote:
> > src/slave/containerizer/provisioners/docker/token_manager.cpp, line 203
> > 
> >
> > We usually name the class variable without _, and the parameter with _.
> 
> Jojy Varghese wrote:
> Not sure I follow.

Never mind we have new style guides now about this.


- Timothy


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


On Aug. 28, 2015, 4:27 a.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37427/
> ---
> 
> (Updated Aug. 28, 2015, 4:27 a.m.)
> 
> 
> Review request for mesos, Lily Chen, Joris Van Remoortere, 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 7b620ff66856b3f0adac121b3297d55ed71a3d99 
>   src/slave/containerizer/provisioners/docker/token_manager.hpp PRE-CREATION 
>   src/slave/containerizer/provisioners/docker/token_manager.cpp PRE-CREATION 
>   src/tests/provisioners/docker_provisioner_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/37427/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>

Re: Review Request 37427: Docker registry: adding TokenManager.

[Timothy Chen]

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



src/slave/containerizer/provisioners/docker/token_manager.cpp (line 124)


This should fit 80 char width.



src/slave/containerizer/provisioners/docker/token_manager.cpp (line 133)


Kill extra space after (



src/slave/containerizer/provisioners/docker/token_manager.cpp (line 223)


If you put the cache here in TokenManager instead of TokenManagerProcess 
then you'll going to be concurrent access of all the fields since it's not 
serialized by libprocess.

I think tokenCache_ will have undefined behavior  when you run at/contains 
while it's being inserted  or deleted then.

I suggest you move all this into the process instead.


- Timothy Chen


On Aug. 28, 2015, 4:27 a.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37427/
> ---
> 
> (Updated Aug. 28, 2015, 4:27 a.m.)
> 
> 
> Review request for mesos, Lily Chen, Joris Van Remoortere, 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 7b620ff66856b3f0adac121b3297d55ed71a3d99 
>   src/slave/containerizer/provisioners/docker/token_manager.hpp PRE-CREATION 
>   src/slave/containerizer/provisioners/docker/token_manager.cpp PRE-CREATION 
>   src/tests/provisioners/docker_provisioner_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/37427/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>

Re: Review Request 37427: Docker registry: adding TokenManager.

[Timothy Chen]

> On Aug. 28, 2015, 12:18 a.m., Timothy Chen wrote:
> > src/slave/containerizer/provisioners/docker/token_manager.cpp, line 78
> > 
> >
> > Captialize first word (Invalid), same as other error messages below.
> 
> Jojy Varghese wrote:
> 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 : "Error to do operation foo: 
> Failed to fetch data".

We don't really follow this practice in Mesos, so if you feel like this is 
something we should do I think you can propose and we can discuss about this.
And you never know who is nesting your error message anyways, so you could also 
end up "Error abc: Error to do op foo: failed to def". For now let's just be 
consistent with all the code base.


> On Aug. 28, 2015, 12:18 a.m., Timothy Chen wrote:
> > src/slave/containerizer/provisioners/docker/token_manager.hpp, line 223
> > 
> >
> > Is there a reason you need to include this in the header? Can we just 
> > put it in cpp?
> 
> Jojy Varghese wrote:
> The only reason being that this is a property of the TokenManager.

I see, does it need to be? Can't we define a static const in the cpp?


- Timothy


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


On Aug. 28, 2015, 4:27 a.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37427/
> ---
> 
> (Updated Aug. 28, 2015, 4:27 a.m.)
> 
> 
> Review request for mesos, Lily Chen, Joris Van Remoortere, 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 7b620ff66856b3f0adac121b3297d55ed71a3d99 
>   src/slave/containerizer/provisioners/docker/token_manager.hpp PRE-CREATION 
>   src/slave/containerizer/provisioners/docker/token_manager.cpp PRE-CREATION 
>   src/tests/provisioners/docker_provisioner_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/37427/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>

Re: Review Request 37427: Docker registry: adding TokenManager.

[Jojy Varghese]

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

(Updated Aug. 28, 2015, 4:27 a.m.)


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


Changes
---

after splitting ssl changes into its own patch.


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 7b620ff66856b3f0adac121b3297d55ed71a3d99 
  src/slave/containerizer/provisioners/docker/token_manager.hpp PRE-CREATION 
  src/slave/containerizer/provisioners/docker/token_manager.cpp PRE-CREATION 
  src/tests/provisioners/docker_provisioner_tests.cpp PRE-CREATION 

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


Testing
---

make check.


Thanks,

Jojy Varghese

Re: Review Request 37427: Docker registry: adding TokenManager.

[Jojy Varghese]

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

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


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


Changes
---

Addressed review comments.


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

  3rdparty/libprocess/include/process/tests/ssl_test.hpp PRE-CREATION 
  3rdparty/libprocess/src/openssl_util.hpp  
  3rdparty/libprocess/src/openssl_util.cpp 
42b2860beccff929563ed05905da226b781918b7 
  3rdparty/libprocess/src/tests/ssl_tests.cpp 
7a316bc10575325ffe732fcc87d72d15a4fc5eaf 
  src/Makefile.am 7b620ff66856b3f0adac121b3297d55ed71a3d99 
  src/slave/containerizer/provisioners/docker/token_manager.hpp PRE-CREATION 
  src/slave/containerizer/provisioners/docker/token_manager.cpp PRE-CREATION 
  src/tests/provisioners/docker_provisioner_tests.cpp PRE-CREATION 

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


Testing
---

make check.


Thanks,

Jojy Varghese

Re: Review Request 37427: Docker registry: adding TokenManager.

[Jojy Varghese]

> On Aug. 28, 2015, 12:18 a.m., Timothy Chen wrote:
> > src/slave/containerizer/provisioners/docker/token_manager.hpp, line 69
> > 
> >
> > is exp and nbf web token fields?

Yes.


> On Aug. 28, 2015, 12:18 a.m., Timothy Chen wrote:
> > src/slave/containerizer/provisioners/docker/token_manager.hpp, line 77
> > 
> >
> > Why keep raw?

Raw is what is used by docker client. Other fields are useful for validation 
etc.


> On Aug. 28, 2015, 12:18 a.m., Timothy Chen wrote:
> > src/slave/containerizer/provisioners/docker/token_manager.hpp, line 223
> > 
> >
> > Is there a reason you need to include this in the header? Can we just 
> > put it in cpp?

The only reason being that this is a property of the TokenManager.


> On Aug. 28, 2015, 12:18 a.m., Timothy Chen wrote:
> > src/slave/containerizer/provisioners/docker/token_manager.cpp, line 78
> > 
> >
> > Captialize first word (Invalid), same as other error messages below.

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 : "Error to do operation foo: 
Failed to fetch data".


> On Aug. 28, 2015, 12:18 a.m., Timothy Chen wrote:
> > src/slave/containerizer/provisioners/docker/token_manager.cpp, line 165
> > 
> >
> > Shouldn't this return true?

No if code does not follow !expired, then the token must be expired and hence 
invalid.


> On Aug. 28, 2015, 12:18 a.m., Timothy Chen wrote:
> > src/slave/containerizer/provisioners/docker/token_manager.cpp, line 203
> > 
> >
> > We usually name the class variable without _, and the parameter with _.

Not sure I follow.


> On Aug. 28, 2015, 12:18 a.m., Timothy Chen wrote:
> > src/slave/containerizer/provisioners/docker/token_manager.cpp, line 164
> > 
> >
> > Kill space

Will fix.


- Jojy


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


On Aug. 26, 2015, 1:03 a.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37427/
> ---
> 
> (Updated Aug. 26, 2015, 1:03 a.m.)
> 
> 
> Review request for mesos, Lily Chen, Joris Van Remoortere, 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 571e1ac0f96b2452797a478680b540f2aab63aab 
>   src/slave/containerizer/provisioners/docker/token_manager.hpp PRE-CREATION 
>   src/slave/containerizer/provisioners/docker/token_manager.cpp PRE-CREATION 
>   src/tests/provisioners/docker_provisioner_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/37427/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>

Re: Review Request 37427: Docker registry: adding TokenManager.

[Timothy Chen]

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



src/slave/containerizer/provisioners/docker/token_manager.hpp (line 69)


is exp and nbf web token fields?



src/slave/containerizer/provisioners/docker/token_manager.hpp (line 77)


Why keep raw?



src/slave/containerizer/provisioners/docker/token_manager.hpp (line 223)


Is there a reason you need to include this in the header? Can we just put 
it in cpp?



src/slave/containerizer/provisioners/docker/token_manager.cpp (line 78)


Captialize first word (Invalid), same as other error messages below.



src/slave/containerizer/provisioners/docker/token_manager.cpp (line 164)


Kill space



src/slave/containerizer/provisioners/docker/token_manager.cpp (line 165)


Shouldn't this return true?



src/slave/containerizer/provisioners/docker/token_manager.cpp (line 203)


We usually name the class variable without _, and the parameter with _.



src/slave/containerizer/provisioners/docker/token_manager.cpp (line 341)


End comment with period.


- Timothy Chen


On Aug. 26, 2015, 1:03 a.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37427/
> ---
> 
> (Updated Aug. 26, 2015, 1:03 a.m.)
> 
> 
> Review request for mesos, Lily Chen, Joris Van Remoortere, 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 571e1ac0f96b2452797a478680b540f2aab63aab 
>   src/slave/containerizer/provisioners/docker/token_manager.hpp PRE-CREATION 
>   src/slave/containerizer/provisioners/docker/token_manager.cpp PRE-CREATION 
>   src/tests/provisioners/docker_provisioner_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/37427/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>

Re: Review Request 37427: Docker registry: adding TokenManager.

[Joris Van Remoortere]

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



src/tests/provisioners/docker_provisioner_tests.cpp (lines 52 - 82)


*IF* we can't generalize the SSL testing class (comment below), then let's 
move openssl_util.hpp into the libprocess public headers instead so we don't 
have to do this.



src/tests/provisioners/docker_provisioner_tests.cpp (lines 229 - 231)


Can you pull `SSLTest` out of `ssl_tests.cpp` into an accessible place so 
you can re-use it?
I think this is worth it right now.


- Joris Van Remoortere


On Aug. 26, 2015, 1:03 a.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37427/
> ---
> 
> (Updated Aug. 26, 2015, 1:03 a.m.)
> 
> 
> Review request for mesos, Lily Chen, Joris Van Remoortere, 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 571e1ac0f96b2452797a478680b540f2aab63aab 
>   src/slave/containerizer/provisioners/docker/token_manager.hpp PRE-CREATION 
>   src/slave/containerizer/provisioners/docker/token_manager.cpp PRE-CREATION 
>   src/tests/provisioners/docker_provisioner_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/37427/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>

Re: Review Request 37427: Docker registry: adding TokenManager.

[Jojy Varghese]

> On Aug. 25, 2015, 10:11 p.m., Lily Chen wrote:
> > src/slave/containerizer/provisioners/docker/token_manager.cpp, line 318
> > 
> >
> > Capitalize beginning of failure messages.

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


- Jojy


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


On Aug. 26, 2015, 1:03 a.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37427/
> ---
> 
> (Updated Aug. 26, 2015, 1:03 a.m.)
> 
> 
> Review request for mesos, Lily Chen, Joris Van Remoortere, 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 571e1ac0f96b2452797a478680b540f2aab63aab 
>   src/slave/containerizer/provisioners/docker/token_manager.hpp PRE-CREATION 
>   src/slave/containerizer/provisioners/docker/token_manager.cpp PRE-CREATION 
>   src/tests/provisioners/docker_provisioner_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/37427/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>

Re: Review Request 37427: Docker registry: adding TokenManager.

[Jojy Varghese]

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

(Updated Aug. 26, 2015, 1:03 a.m.)


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


Changes
---

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 571e1ac0f96b2452797a478680b540f2aab63aab 
  src/slave/containerizer/provisioners/docker/token_manager.hpp PRE-CREATION 
  src/slave/containerizer/provisioners/docker/token_manager.cpp PRE-CREATION 
  src/tests/provisioners/docker_provisioner_tests.cpp PRE-CREATION 

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


Testing
---

make check.


Thanks,

Jojy Varghese

Re: Review Request 37427: Docker registry: adding TokenManager.

[Lily Chen]

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



src/Makefile.am (line 477)


Group token_manager.hpp with other provisioner header files, see lines 
740-744.



src/slave/containerizer/provisioners/docker/token_manager.hpp (lines 22 - 23)


Make sure included libraries are in alphabetical order.



src/slave/containerizer/provisioners/docker/token_manager.hpp (line 26)


Same here (libraries in alphabetical order).



src/slave/containerizer/provisioners/docker/token_manager.hpp (lines 30 - 33)


Stout libraries are usually included before process libraries



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


Name parameters.



src/slave/containerizer/provisioners/docker/token_manager.hpp (line 96)


Period at the end of comment.



src/slave/containerizer/provisioners/docker/token_manager.hpp (line 97)


classes are separated by two empty lines.



src/slave/containerizer/provisioners/docker/token_manager.hpp (line 198)


name parameter.



src/slave/containerizer/provisioners/docker/token_manager.hpp (line 213)


place member variables after function declarations.



src/slave/containerizer/provisioners/docker/token_manager.cpp (lines 57 - 70)


Why not just use a private member function?



src/slave/containerizer/provisioners/docker/token_manager.cpp (lines 217 - 219)


Should only be indented 4 spaces.



src/slave/containerizer/provisioners/docker/token_manager.cpp (lines 249 - 251)


Indent 2 more spaces



src/slave/containerizer/provisioners/docker/token_manager.cpp (lines 254 - 255)


Indent by 2 spaces



src/slave/containerizer/provisioners/docker/token_manager.cpp (line 318)


Capitalize beginning of failure messages.



src/slave/containerizer/provisioners/docker/token_manager.cpp (line 321)


indent 2 more spaces?



src/slave/containerizer/provisioners/docker/token_manager.cpp (line 323)


Here too.



src/slave/containerizer/provisioners/docker/token_manager.cpp (line 330)


here too.



src/slave/containerizer/provisioners/docker/token_manager.cpp (line 341)


Fix implementatio typo and period at the end of comment.



src/tests/provisioners/docker_provisioner_tests.cpp (line 562)


extra space here.


- Lily Chen


On Aug. 24, 2015, 5:16 p.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37427/
> ---
> 
> (Updated Aug. 24, 2015, 5:16 p.m.)
> 
> 
> Review request for mesos, Lily Chen, Joris Van Remoortere, 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 9fd71d1ddf442712977596e7a13969ff5c1d68db 
>   src/slave/containerizer/provisioners/docker/token_manager.hpp PRE-CREATION 
>   src/slave/containerizer/provisioners/docker/token_manager.cpp PRE-CREATION 
>   src/tests/provisioners/docker_provisioner_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/37427/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>

Re: Review Request 37427: Docker registry: adding TokenManager.

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [37426, 37427]

All tests passed.

- Mesos ReviewBot


On Aug. 24, 2015, 5:16 p.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37427/
> ---
> 
> (Updated Aug. 24, 2015, 5:16 p.m.)
> 
> 
> Review request for mesos, Lily Chen, Joris Van Remoortere, 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 9fd71d1ddf442712977596e7a13969ff5c1d68db 
>   src/slave/containerizer/provisioners/docker/token_manager.hpp PRE-CREATION 
>   src/slave/containerizer/provisioners/docker/token_manager.cpp PRE-CREATION 
>   src/tests/provisioners/docker_provisioner_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/37427/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>

Re: Review Request 37427: Docker registry: adding TokenManager.

[Jojy Varghese]

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

(Updated Aug. 24, 2015, 5:16 p.m.)


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


Changes
---

more style changes.


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 9fd71d1ddf442712977596e7a13969ff5c1d68db 
  src/slave/containerizer/provisioners/docker/token_manager.hpp PRE-CREATION 
  src/slave/containerizer/provisioners/docker/token_manager.cpp PRE-CREATION 
  src/tests/provisioners/docker_provisioner_tests.cpp PRE-CREATION 

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


Testing
---

make check.


Thanks,

Jojy Varghese

Re: Review Request 37427: Docker registry: adding TokenManager.

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [37426, 37427]

All tests passed.

- Mesos ReviewBot


On Aug. 20, 2015, 3:50 p.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37427/
> ---
> 
> (Updated Aug. 20, 2015, 3:50 p.m.)
> 
> 
> Review request for mesos, Lily Chen, Joris Van Remoortere, 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 9fd71d1ddf442712977596e7a13969ff5c1d68db 
>   src/slave/containerizer/provisioners/docker/token_manager.hpp PRE-CREATION 
>   src/slave/containerizer/provisioners/docker/token_manager.cpp PRE-CREATION 
>   src/tests/provisioners/docker_provisioner_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/37427/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>

Re: Review Request 37427: Docker registry: adding TokenManager.

[Jojy Varghese]

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

(Updated Aug. 20, 2015, 3:50 p.m.)


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


Changes
---

rebased with master


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 9fd71d1ddf442712977596e7a13969ff5c1d68db 
  src/slave/containerizer/provisioners/docker/token_manager.hpp PRE-CREATION 
  src/slave/containerizer/provisioners/docker/token_manager.cpp PRE-CREATION 
  src/tests/provisioners/docker_provisioner_tests.cpp PRE-CREATION 

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


Testing
---

make check.


Thanks,

Jojy Varghese

Re: Review Request 37427: Docker registry: adding TokenManager.

[Mesos ReviewBot]

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


Bad patch!

Reviews applied: [37426, 37427]

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

Error:
 2015-08-20 00:18:52 URL:https://reviews.apache.org/r/37427/diff/raw/ 
[34162/34162] -> "37427.patch" [1]
error: patch failed: src/Makefile.am:472
error: src/Makefile.am: patch does not apply
Failed to apply patch

- Mesos ReviewBot


On Aug. 19, 2015, 5:25 p.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37427/
> ---
> 
> (Updated Aug. 19, 2015, 5:25 p.m.)
> 
> 
> Review request for mesos, Lily Chen, Joris Van Remoortere, 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 457ad26ee55bd7a2aedf27f45db58a9a4a6a5dc5 
>   src/slave/containerizer/provisioners/docker/token_manager.hpp PRE-CREATION 
>   src/slave/containerizer/provisioners/docker/token_manager.cpp PRE-CREATION 
>   src/tests/provisioners/docker_provisioner_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/37427/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>

Re: Review Request 37427: Docker registry: adding TokenManager.

[Jojy Varghese]

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

(Updated Aug. 19, 2015, 5:25 p.m.)


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


Changes
---

rebased with master.


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 457ad26ee55bd7a2aedf27f45db58a9a4a6a5dc5 
  src/slave/containerizer/provisioners/docker/token_manager.hpp PRE-CREATION 
  src/slave/containerizer/provisioners/docker/token_manager.cpp PRE-CREATION 
  src/tests/provisioners/docker_provisioner_tests.cpp PRE-CREATION 

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


Testing
---

make check.


Thanks,

Jojy Varghese

Re: Review Request 37427: Docker registry: adding TokenManager.

[Jojy Varghese]

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

(Updated Aug. 18, 2015, 11:24 p.m.)


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


Changes
---

style changes.


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 457ad26ee55bd7a2aedf27f45db58a9a4a6a5dc5 
  src/slave/containerizer/provisioners/docker/token_manager.hpp PRE-CREATION 
  src/slave/containerizer/provisioners/docker/token_manager.cpp PRE-CREATION 
  src/tests/provisioners/docker_provisioner_tests.cpp PRE-CREATION 

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


Testing
---

make check.


Thanks,

Jojy Varghese

Re: Review Request 37427: Docker registry: adding TokenManager.

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [37426, 37427]

All tests passed.

- Mesos ReviewBot


On Aug. 14, 2015, 5:28 p.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37427/
> ---
> 
> (Updated Aug. 14, 2015, 5:28 p.m.)
> 
> 
> Review request for mesos, Lily Chen, Joris Van Remoortere, 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 e990369139e7ac3b86f8b04cfd5bef559e16dd24 
>   src/slave/containerizer/provisioners/docker/token_manager.hpp PRE-CREATION 
>   src/slave/containerizer/provisioners/docker/token_manager.cpp PRE-CREATION 
>   src/tests/provisioners/docker_provisioner_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/37427/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>

Re: Review Request 37427: Docker registry: adding TokenManager.

[Jojy Varghese]

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

(Updated Aug. 14, 2015, 5:28 p.m.)


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


Changes
---

fixing build part 2.


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 e990369139e7ac3b86f8b04cfd5bef559e16dd24 
  src/slave/containerizer/provisioners/docker/token_manager.hpp PRE-CREATION 
  src/slave/containerizer/provisioners/docker/token_manager.cpp PRE-CREATION 
  src/tests/provisioners/docker_provisioner_tests.cpp PRE-CREATION 

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


Testing
---

make check.


Thanks,

Jojy Varghese

Re: Review Request 37427: Docker registry: adding TokenManager.

[Jojy Varghese]

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

(Updated Aug. 14, 2015, 2:43 p.m.)


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


Changes
---

build fix.


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 e990369139e7ac3b86f8b04cfd5bef559e16dd24 
  src/slave/containerizer/provisioners/docker/token_manager.hpp PRE-CREATION 
  src/slave/containerizer/provisioners/docker/token_manager.cpp PRE-CREATION 
  src/tests/provisioners/docker_provisioner_tests.cpp PRE-CREATION 

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


Testing
---

make check.


Thanks,

Jojy Varghese

Re: Review Request 37427: Docker registry: adding TokenManager.

[Jojy Varghese]

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

(Updated Aug. 14, 2015, 12:09 a.m.)


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


Changes
---

added more tests.


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 b5dbdc316cad179cd265bd81900999fab35940b9 
  src/slave/containerizer/provisioners/docker/token_manager.hpp PRE-CREATION 
  src/slave/containerizer/provisioners/docker/token_manager.cpp PRE-CREATION 
  src/tests/provisioners/docker_provisioner_tests.cpp PRE-CREATION 

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


Testing
---

make check.


Thanks,

Jojy Varghese

Re: Review Request 37427: Docker registry: adding TokenManager.

[Jojy Varghese]

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

(Updated Aug. 13, 2015, 10:26 p.m.)


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


Changes
---

addressed review comments.


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 b5dbdc316cad179cd265bd81900999fab35940b9 
  src/slave/containerizer/provisioners/docker/token_manager.hpp PRE-CREATION 
  src/slave/containerizer/provisioners/docker/token_manager.cpp PRE-CREATION 
  src/tests/provisioners/docker_provisioner_tests.cpp PRE-CREATION 

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


Testing
---

make check.


Thanks,

Jojy Varghese

Re: Review Request 37427: Docker registry: adding TokenManager.

[Timothy Chen]

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



src/slave/containerizer/provisioners/docker/token_manager.hpp (line 48)


Move class variables after the method declarations



src/slave/containerizer/provisioners/docker/token_manager.hpp (line 82)


Name the parameters,
also 4 space indent and one parameter each line.



src/slave/containerizer/provisioners/docker/token_manager.hpp (line 93)


Nit: I think we usually comment Forward declarations (although I don't 
think it's in the style guide)



src/slave/containerizer/provisioners/docker/token_manager.cpp (line 120)


Let's not use auto here, spell out the type.



src/slave/containerizer/provisioners/docker/token_manager.cpp (line 122)


This should fit in one line



src/tests/containerizer/docker_containerizer_tests.cpp (line 86)


Add a TODO around these that we can move this into a common test SSL server 
utility.



src/tests/containerizer/docker_containerizer_tests.cpp (line 2998)


Let's just create a new Docker provisioner test file for this.


- Timothy Chen


On Aug. 13, 2015, 8:55 p.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:55 p.m.)
> 
> 
> Review request for mesos, Lily Chen, Joris Van Remoortere, 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.

[Jojy Varghese]

> On Aug. 13, 2015, 6:24 a.m., Lily Chen wrote:
> > src/slave/containerizer/provisioners/docker/token_manager.cpp, line 252
> > 
> >
> > 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.

[Jojy Varghese]

> On Aug. 13, 2015, 6:24 a.m., Lily Chen wrote:
> > src/slave/containerizer/provisioners/docker/token_manager.cpp, lines 37-46
> > 
> >
> > 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
> > 
> >
> > 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.

[Mesos ReviewBot]

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

[Jojy Varghese]

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

[Lily Chen]

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


space between // and TODO



src/slave/containerizer/provisioners/docker/token_manager.hpp (line 53)


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)


no need to put underscore in front of parameters being passed in



src/slave/containerizer/provisioners/docker/token_manager.cpp (line 135)


space between ) and {



src/slave/containerizer/provisioners/docker/token_manager.cpp (line 230)


pass in token by const reference?



src/slave/containerizer/provisioners/docker/token_manager.cpp (line 252)


space between () {}, you could also probably just do this in the header file



src/tests/containerizer/docker_containerizer_tests.cpp (line 2998)


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.

[Jojy Varghese]

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

[Mesos ReviewBot]

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