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

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

    Extra space ?



src/Makefile.am (line 758)
<https://reviews.apache.org/r/37427/#comment153408>

    Ditto. Same for the other 3 occurences



src/slave/containerizer/provisioners/docker/token_manager.hpp (line 19)
<https://reviews.apache.org/r/37427/#comment153411>

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

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

    



src/slave/containerizer/provisioners/docker/token_manager.hpp (line 79)
<https://reviews.apache.org/r/37427/#comment153439>

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



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

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



src/slave/containerizer/provisioners/docker/token_manager.hpp (line 88)
<https://reviews.apache.org/r/37427/#comment153422>

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



src/slave/containerizer/provisioners/docker/token_manager.hpp (line 104)
<https://reviews.apache.org/r/37427/#comment153423>

    Space after TODO



src/slave/containerizer/provisioners/docker/token_manager.hpp (line 116)
<https://reviews.apache.org/r/37427/#comment153424>

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

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

    Same as above ?



src/slave/containerizer/provisioners/docker/token_manager.cpp (line 48)
<https://reviews.apache.org/r/37427/#comment153433>

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



src/slave/containerizer/provisioners/docker/token_manager.cpp (line 120)
<https://reviews.apache.org/r/37427/#comment153435>

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

    Time notBefore; ?



src/slave/containerizer/provisioners/docker/token_manager.cpp (line 175)
<https://reviews.apache.org/r/37427/#comment153438>

    Indent



src/slave/containerizer/provisioners/docker/token_manager.cpp (line 205)
<https://reviews.apache.org/r/37427/#comment153441>

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



src/slave/containerizer/provisioners/docker/token_manager.cpp (line 278)
<https://reviews.apache.org/r/37427/#comment153443>

    Nit: s/tokenString/token ?



src/slave/containerizer/provisioners/docker/token_manager.cpp (line 286)
<https://reviews.apache.org/r/37427/#comment153445>

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

    Nit: Why not just key ?



src/slave/containerizer/provisioners/docker/token_manager.cpp (line 308)
<https://reviews.apache.org/r/37427/#comment153450>

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



src/slave/containerizer/provisioners/docker/token_manager.cpp (line 325)
<https://reviews.apache.org/r/37427/#comment153455>

    Nit: s/resp/response



src/tests/provisioners/docker_provisioner_tests.cpp (line 29)
<https://reviews.apache.org/r/37427/#comment153461>

    New line here



src/tests/provisioners/docker_provisioner_tests.cpp (line 31)
<https://reviews.apache.org/r/37427/#comment153456>

    stout includes go before process



src/tests/provisioners/docker_provisioner_tests.cpp (line 34)
<https://reviews.apache.org/r/37427/#comment153457>

    Newline before



src/tests/provisioners/docker_provisioner_tests.cpp (line 41)
<https://reviews.apache.org/r/37427/#comment153460>

    As above, selectively include from process. Same for below line.


- Anand Mazumdar


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

Reply via email to