You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mesos.apache.org by Vinod Kone <vi...@gmail.com> on 2014/06/02 20:01:55 UTC
Review Request 22150: Injected Authorizer into Master.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/22150/
-----------------------------------------------------------
Review request for mesos and Ben Mahler.
Repository: mesos-git
Description
-------
Injected the Authorizer so that I can write more interesting tests.
Diffs
-----
src/local/local.cpp 5d26afffcdca0eb9d19499564a8edd2bf3dc1e66
src/master/main.cpp 8ceaae61a732a8d68d19c8aee97c1fd93a595893
src/master/master.hpp d4ef4bec7168179f2168e88d3727e50b0e2e68a1
src/master/master.cpp 766a0e36a6e7a615e7b2974d9fee70bcef446719
src/tests/cluster.hpp f4cc9a62cd0ca86cd87987d963abd951a377ddd1
Diff: https://reviews.apache.org/r/22150/diff/
Testing
-------
make check
Thanks,
Vinod Kone
Re: Review Request 22150: Injected Authorizer into Master.
Posted by Vinod Kone <vi...@gmail.com>.
> On June 2, 2014, 11:35 p.m., Ben Mahler wrote:
> > src/local/local.cpp, lines 171-172
> > <https://reviews.apache.org/r/22150/diff/1/?file=601784#file601784line171>
> >
> > Do you need the temporary or can this just be:
> >
> > authorizer = authorizer_.get().release();
Owned::get() gives a const reference on which you can't call release().
> On June 2, 2014, 11:35 p.m., Ben Mahler wrote:
> > src/master/main.cpp, lines 257-258
> > <https://reviews.apache.org/r/22150/diff/1/?file=601785#file601785line257>
> >
> > Ditto here.
see above.
> On June 2, 2014, 11:35 p.m., Ben Mahler wrote:
> > src/master/master.cpp, line 31
> > <https://reviews.apache.org/r/22150/diff/1/?file=601787#file601787line31>
> >
> > Curious why you decided to add the 'owned' include even though there are no new Owned things in this change. At least we should include 'option' then too?
it was an artifact of authorizer not being injected in the first pass. but, the master code still uses owned (like registrar :)) so the header should be included here. also included option.
> On June 2, 2014, 11:35 p.m., Ben Mahler wrote:
> > src/tests/cluster.hpp, lines 364-365
> > <https://reviews.apache.org/r/22150/diff/1/?file=601788#file601788line364>
> >
> > Ditto here.
see above
- Vinod
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/22150/#review44573
-----------------------------------------------------------
On June 2, 2014, 6:01 p.m., Vinod Kone wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/22150/
> -----------------------------------------------------------
>
> (Updated June 2, 2014, 6:01 p.m.)
>
>
> Review request for mesos and Ben Mahler.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Injected the Authorizer so that I can write more interesting tests.
>
>
> Diffs
> -----
>
> src/local/local.cpp 5d26afffcdca0eb9d19499564a8edd2bf3dc1e66
> src/master/main.cpp 8ceaae61a732a8d68d19c8aee97c1fd93a595893
> src/master/master.hpp d4ef4bec7168179f2168e88d3727e50b0e2e68a1
> src/master/master.cpp 766a0e36a6e7a615e7b2974d9fee70bcef446719
> src/tests/cluster.hpp f4cc9a62cd0ca86cd87987d963abd951a377ddd1
>
> Diff: https://reviews.apache.org/r/22150/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Vinod Kone
>
>
Re: Review Request 22150: Injected Authorizer into Master.
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/22150/#review44573
-----------------------------------------------------------
Ship it!
src/local/local.cpp
<https://reviews.apache.org/r/22150/#comment78989>
Do you need the temporary or can this just be:
authorizer = authorizer_.get().release();
src/master/main.cpp
<https://reviews.apache.org/r/22150/#comment78991>
Add the option header include?
src/master/main.cpp
<https://reviews.apache.org/r/22150/#comment78990>
Ditto here.
src/master/master.cpp
<https://reviews.apache.org/r/22150/#comment78994>
Curious why you decided to add the 'owned' include even though there are no new Owned things in this change. At least we should include 'option' then too?
src/master/master.cpp
<https://reviews.apache.org/r/22150/#comment78992>
"Authorization is enabled"?
src/tests/cluster.hpp
<https://reviews.apache.org/r/22150/#comment78993>
Ditto here.
- Ben Mahler
On June 2, 2014, 6:01 p.m., Vinod Kone wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/22150/
> -----------------------------------------------------------
>
> (Updated June 2, 2014, 6:01 p.m.)
>
>
> Review request for mesos and Ben Mahler.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Injected the Authorizer so that I can write more interesting tests.
>
>
> Diffs
> -----
>
> src/local/local.cpp 5d26afffcdca0eb9d19499564a8edd2bf3dc1e66
> src/master/main.cpp 8ceaae61a732a8d68d19c8aee97c1fd93a595893
> src/master/master.hpp d4ef4bec7168179f2168e88d3727e50b0e2e68a1
> src/master/master.cpp 766a0e36a6e7a615e7b2974d9fee70bcef446719
> src/tests/cluster.hpp f4cc9a62cd0ca86cd87987d963abd951a377ddd1
>
> Diff: https://reviews.apache.org/r/22150/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Vinod Kone
>
>
Re: Review Request 22150: Injected Authorizer into Master.
Posted by Vinod Kone <vi...@gmail.com>.
> On June 2, 2014, 6:39 p.m., Dominic Hamon wrote:
> > src/master/master.hpp, line 103
> > <https://reviews.apache.org/r/22150/diff/1/?file=601786#file601786line103>
> >
> > what's the conceptual difference between an Option<T*> and a T* that can be NULL?
using an option makes it explicit that the pointer might not be set. this will help us differentiate with the case when the pointer shouldn't exist but shouldn't be NULL. we've used this pattern elsewhere in the code.
> On June 2, 2014, 6:39 p.m., Dominic Hamon wrote:
> > src/master/main.cpp, line 271
> > <https://reviews.apache.org/r/22150/diff/1/?file=601785#file601785line271>
> >
> > ditto re Owned::get
I didn't inject an Owned pointer to be consistent with how everything else is injected (registrar, allocator, etc) into the master.
> On June 2, 2014, 6:39 p.m., Dominic Hamon wrote:
> > src/local/local.cpp, line 182
> > <https://reviews.apache.org/r/22150/diff/1/?file=601784#file601784line182>
> >
> > it might be better to keep authorizer as Owned and pass in authorizer.get() here. Then you don't need the explicit authorizer delete below.
see below.
- Vinod
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/22150/#review44524
-----------------------------------------------------------
On June 2, 2014, 6:01 p.m., Vinod Kone wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/22150/
> -----------------------------------------------------------
>
> (Updated June 2, 2014, 6:01 p.m.)
>
>
> Review request for mesos and Ben Mahler.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Injected the Authorizer so that I can write more interesting tests.
>
>
> Diffs
> -----
>
> src/local/local.cpp 5d26afffcdca0eb9d19499564a8edd2bf3dc1e66
> src/master/main.cpp 8ceaae61a732a8d68d19c8aee97c1fd93a595893
> src/master/master.hpp d4ef4bec7168179f2168e88d3727e50b0e2e68a1
> src/master/master.cpp 766a0e36a6e7a615e7b2974d9fee70bcef446719
> src/tests/cluster.hpp f4cc9a62cd0ca86cd87987d963abd951a377ddd1
>
> Diff: https://reviews.apache.org/r/22150/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Vinod Kone
>
>
Re: Review Request 22150: Injected Authorizer into Master.
Posted by Dominic Hamon <dh...@twopensource.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/22150/#review44524
-----------------------------------------------------------
src/local/local.cpp
<https://reviews.apache.org/r/22150/#comment78935>
it might be better to keep authorizer as Owned and pass in authorizer.get() here. Then you don't need the explicit authorizer delete below.
src/master/main.cpp
<https://reviews.apache.org/r/22150/#comment78936>
ditto re Owned::get
src/master/master.hpp
<https://reviews.apache.org/r/22150/#comment78937>
what's the conceptual difference between an Option<T*> and a T* that can be NULL?
- Dominic Hamon
On June 2, 2014, 11:01 a.m., Vinod Kone wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/22150/
> -----------------------------------------------------------
>
> (Updated June 2, 2014, 11:01 a.m.)
>
>
> Review request for mesos and Ben Mahler.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Injected the Authorizer so that I can write more interesting tests.
>
>
> Diffs
> -----
>
> src/local/local.cpp 5d26afffcdca0eb9d19499564a8edd2bf3dc1e66
> src/master/main.cpp 8ceaae61a732a8d68d19c8aee97c1fd93a595893
> src/master/master.hpp d4ef4bec7168179f2168e88d3727e50b0e2e68a1
> src/master/master.cpp 766a0e36a6e7a615e7b2974d9fee70bcef446719
> src/tests/cluster.hpp f4cc9a62cd0ca86cd87987d963abd951a377ddd1
>
> Diff: https://reviews.apache.org/r/22150/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Vinod Kone
>
>
Re: Review Request 22150: Injected Authorizer into Master.
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/22150/
-----------------------------------------------------------
(Updated June 3, 2014, 11 p.m.)
Review request for mesos and Ben Mahler.
Changes
-------
rebased.
Repository: mesos-git
Description
-------
Injected the Authorizer so that I can write more interesting tests.
Diffs (updated)
-----
src/local/local.cpp 5d26afffcdca0eb9d19499564a8edd2bf3dc1e66
src/master/main.cpp 8ceaae61a732a8d68d19c8aee97c1fd93a595893
src/master/master.hpp d4ef4bec7168179f2168e88d3727e50b0e2e68a1
src/master/master.cpp 766a0e36a6e7a615e7b2974d9fee70bcef446719
src/tests/cluster.hpp f4cc9a62cd0ca86cd87987d963abd951a377ddd1
Diff: https://reviews.apache.org/r/22150/diff/
Testing
-------
make check
Thanks,
Vinod Kone