mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Jojy Varghese" <j...@mesosphere.io>
Subject Re: Review Request 37427: Docker registry: adding TokenManager.
Date Wed, 02 Sep 2015 21:01:19 GMT


> 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
> > <https://reviews.apache.org/r/37427/diff/18/?file=1062396#file1062396line484>
> >
> >     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
> > <https://reviews.apache.org/r/37427/diff/18/?file=1062396#file1062396line761>
> >
> >     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
> > <https://reviews.apache.org/r/37427/diff/18/?file=1062397#file1062397line19>
> >
> >     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
> > <https://reviews.apache.org/r/37427/diff/18/?file=1062397#file1062397line79>
> >
> >     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
> > <https://reviews.apache.org/r/37427/diff/18/?file=1062397#file1062397line82>
> >
> >     Token(
> >         const std::string& raw,
> >         const JSON::Object& headerJson,
> >         const JSON::Object& claimsJson,
> >         const Option<process::Time>& expireTime,
> >         const Option<process::Time>& 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
> > <https://reviews.apache.org/r/37427/diff/18/?file=1062397#file1062397line88>
> >
> >     static Result<process::Time> 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
> > <https://reviews.apache.org/r/37427/diff/18/?file=1062397#file1062397line104>
> >
> >     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
> > <https://reviews.apache.org/r/37427/diff/18/?file=1062397#file1062397line116>
> >
> >     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
> > <https://reviews.apache.org/r/37427/diff/18/?file=1062398#file1062398line24>
> >
> >     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
> > <https://reviews.apache.org/r/37427/diff/18/?file=1062398#file1062398line25>
> >
> >     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
> > <https://reviews.apache.org/r/37427/diff/18/?file=1062398#file1062398line167>
> >
> >     Time notBefore; ?

No i meant option<time>.


> On Sept. 2, 2015, 8:01 p.m., Anand Mazumdar wrote:
> > src/slave/containerizer/provisioners/docker/token_manager.cpp, line 205
> > <https://reviews.apache.org/r/37427/diff/18/?file=1062398#file1062398line205>
> >
> >     How about just :
> >     
> >     return expiration.isSome() && Clock::now() >= expiration.get() ?

for readability.


> On Sept. 2, 2015, 8:01 p.m., Anand Mazumdar wrote:
> > src/slave/containerizer/provisioners/docker/token_manager.cpp, line 278
> > <https://reviews.apache.org/r/37427/diff/18/?file=1062398#file1062398line278>
> >
> >     Nit: s/tokenString/token ?

No i meant tokenString.


> On Sept. 2, 2015, 8:01 p.m., Anand Mazumdar wrote:
> > src/slave/containerizer/provisioners/docker/token_manager.cpp, line 300
> > <https://reviews.apache.org/r/37427/diff/18/?file=1062398#file1062398line300>
> >
> >     Nit: Why not just key ?

to be explicit.


> On Sept. 2, 2015, 8:01 p.m., Anand Mazumdar wrote:
> > src/slave/containerizer/provisioners/docker/token_manager.cpp, line 325
> > <https://reviews.apache.org/r/37427/diff/18/?file=1062398#file1062398line325>
> >
> >     Nit: s/resp/response

in the interest of 80 chars. And also because resp is used in only 1 line. So there is no
ambiguity issue.


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


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message