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