> 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. > > 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 > > <https://reviews.apache.org/r/37427/diff/18/?file=1062396#file1062396line761> > > > > 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 > > <https://reviews.apache.org/r/37427/diff/18/?file=1062397#file1062397line79> > > > > 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 > > <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); > > 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 > > <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 ? > > 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 > > <https://reviews.apache.org/r/37427/diff/18/?file=1062397#file1062397line104> > > > > 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 > > <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 ? > > 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 > > <https://reviews.apache.org/r/37427/diff/18/?file=1062398#file1062398line167> > > > > Time notBefore; ? > > Jojy Varghese wrote: > No i meant option<time>. 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 > > <https://reviews.apache.org/r/37427/diff/18/?file=1062398#file1062398line300> > > > > 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 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 > >
