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