mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Jojy Varghese" <j...@mesosphere.io>
Subject Re: Review Request 37427: Docker registry: adding TokenManager.
Date Wed, 02 Sep 2015 21:58:13 GMT


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


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message