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]

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

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.

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

[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: 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/#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.

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

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

 On Aug. 28, 2015, 12:18 a.m., Timothy Chen wrote:
  src/slave/containerizer/provisioners/docker/token_manager.hpp, line 223
  https://reviews.apache.org/r/37427/diff/12/?file=1052806#file1052806line223
 
  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
  https://reviews.apache.org/r/37427/diff/14/?file=1057180#file1057180line223
 
  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.

[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 69
  https://reviews.apache.org/r/37427/diff/12/?file=1052806#file1052806line69
 
  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
  https://reviews.apache.org/r/37427/diff/12/?file=1052806#file1052806line77
 
  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
  https://reviews.apache.org/r/37427/diff/12/?file=1052806#file1052806line223
 
  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
  https://reviews.apache.org/r/37427/diff/12/?file=1052807#file1052807line78
 
  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
  https://reviews.apache.org/r/37427/diff/12/?file=1052807#file1052807line165
 
  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
  https://reviews.apache.org/r/37427/diff/12/?file=1052807#file1052807line203
 
  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
  https://reviews.apache.org/r/37427/diff/12/?file=1052807#file1052807line164
 
  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.

[Lily Chen]

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



src/Makefile.am (line 477)
https://reviews.apache.org/r/37427/#comment151767

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



src/slave/containerizer/provisioners/docker/token_manager.hpp (lines 22 - 23)
https://reviews.apache.org/r/37427/#comment151768

Make sure included libraries are in alphabetical order.



src/slave/containerizer/provisioners/docker/token_manager.hpp (line 26)
https://reviews.apache.org/r/37427/#comment151769

Same here (libraries in alphabetical order).



src/slave/containerizer/provisioners/docker/token_manager.hpp (lines 30 - 33)
https://reviews.apache.org/r/37427/#comment151770

Stout libraries are usually included before process libraries



src/slave/containerizer/provisioners/docker/token_manager.hpp (line 57)
https://reviews.apache.org/r/37427/#comment151771

Name parameters.



src/slave/containerizer/provisioners/docker/token_manager.hpp (line 96)
https://reviews.apache.org/r/37427/#comment151773

Period at the end of comment.



src/slave/containerizer/provisioners/docker/token_manager.hpp (line 97)
https://reviews.apache.org/r/37427/#comment151772

classes are separated by two empty lines.



src/slave/containerizer/provisioners/docker/token_manager.hpp (line 198)
https://reviews.apache.org/r/37427/#comment151774

name parameter.



src/slave/containerizer/provisioners/docker/token_manager.hpp (line 213)
https://reviews.apache.org/r/37427/#comment151797

place member variables after function declarations.



src/slave/containerizer/provisioners/docker/token_manager.cpp (lines 57 - 70)
https://reviews.apache.org/r/37427/#comment151776

Why not just use a private member function?



src/slave/containerizer/provisioners/docker/token_manager.cpp (lines 217 - 219)
https://reviews.apache.org/r/37427/#comment151779

Should only be indented 4 spaces.



src/slave/containerizer/provisioners/docker/token_manager.cpp (lines 249 - 251)
https://reviews.apache.org/r/37427/#comment151792

Indent 2 more spaces



src/slave/containerizer/provisioners/docker/token_manager.cpp (lines 254 - 255)
https://reviews.apache.org/r/37427/#comment151793

Indent by 2 spaces



src/slave/containerizer/provisioners/docker/token_manager.cpp (line 318)
https://reviews.apache.org/r/37427/#comment151786

Capitalize beginning of failure messages.



src/slave/containerizer/provisioners/docker/token_manager.cpp (line 321)
https://reviews.apache.org/r/37427/#comment151790

indent 2 more spaces?



src/slave/containerizer/provisioners/docker/token_manager.cpp (line 323)
https://reviews.apache.org/r/37427/#comment151787

Here too.



src/slave/containerizer/provisioners/docker/token_manager.cpp (line 330)
https://reviews.apache.org/r/37427/#comment151789

here too.



src/slave/containerizer/provisioners/docker/token_manager.cpp (line 341)
https://reviews.apache.org/r/37427/#comment151791

Fix implementatio typo and period at the end of comment.



src/tests/provisioners/docker_provisioner_tests.cpp (line 562)
https://reviews.apache.org/r/37427/#comment151799

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.

[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/#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. 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/#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.

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

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

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

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

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

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

 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.

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

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

[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)
https://reviews.apache.org/r/37427/#comment150235

Move class variables after the method declarations



src/slave/containerizer/provisioners/docker/token_manager.hpp (line 82)
https://reviews.apache.org/r/37427/#comment150231

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



src/slave/containerizer/provisioners/docker/token_manager.hpp (line 93)
https://reviews.apache.org/r/37427/#comment150232

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)
https://reviews.apache.org/r/37427/#comment150237

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



src/slave/containerizer/provisioners/docker/token_manager.cpp (line 122)
https://reviews.apache.org/r/37427/#comment150236

This should fit in one line



src/tests/containerizer/docker_containerizer_tests.cpp (line 86)
https://reviews.apache.org/r/37427/#comment150239

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)
https://reviews.apache.org/r/37427/#comment150238

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

[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
 

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