> 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).
> 
> Anand Mazumdar wrote:
>     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 ?

I must be misunderstanding what you are trying to say. google style guide asks 
to have the header guard to follow directory path. Here the dierctory path is 
too deep. So the relative path is slave/containerizer. Then the rest of the 
path is provisioner/docker. We dont follow that in any provisioner headers now. 
The uniqueness about this header is that it is a "provisioner" of type "docker" 
and belongs to the "registry" namespace. I have the extra "registry" in the 
guard to reflect that.


> 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.
> 
> Anand Mazumdar wrote:
>     Looks pretty redundant here. "key" can refer to only one thing inside 
> this function and that being a "tokenKey"

:). Its a personal choice.


> 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.
> 
> Anand Mazumdar wrote:
>     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 ?

I understand namespace pollution issue but its a common pattern i saw in most 
of the cpp files.


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

Reply via email to