> On March 8, 2016, 1:08 a.m., Vinod Kone wrote:
> > include/mesos/authorizer/authorizer.hpp, line 58
> > <https://reviews.apache.org/r/44319/diff/3/?file=1282986#file1282986line58>
> >
> >     seems weird that this interface takes ACLs as a param. can we make it 
> > take Parameters instead? I think it gets rid of a bunch of boilerplate code 
> > below.
> 
> Alexander Rojas wrote:
>     It won't get rid of any boiler plate code, and I will actually have to 
> add some extra code to translate flags to acls, since the place where this 
> constructor is used is done as `Try<Authorizer*> create = 
> Authorizer::create(authorizerName, flags.acls);`, Not that this works mostly 
> as a dispatcher which will either call `LocalAuthorizer::create(const ACLs& 
> acls)` or `modules::ModuleManager::create<Authorizer>(name);`.
>     
>     I considered using a constructor which looks as `Authorizer::create(const 
> Flags&)` but the flags are `internal` which means they are not part of the 
> exported headers.
>     
>     However, I will change it to parameters mostly for flexibility! since 
> this header doesn't belong neither to master nor to the agent but can be used 
> by both.

I see. Lets add a separate overload for Authorizer::create() that takes ACLs 
then. See my comments in the next review.


> On March 8, 2016, 1:08 a.m., Vinod Kone wrote:
> > src/examples/test_authorizer_module.cpp, line 41
> > <https://reviews.apache.org/r/44319/diff/3/?file=1282990#file1282990line41>
> >
> >     s/rawAcls/stringAcls/

actually, lets call this "acls_".


> On March 8, 2016, 1:08 a.m., Vinod Kone wrote:
> > src/tests/authorization_tests.cpp, line 51
> > <https://reviews.apache.org/r/44319/diff/3/?file=1282993#file1282993line51>
> >
> >     have we used this pattern elsewhere?
> 
> Alexander Rojas wrote:
>     We have a similar patter with the 
> [`http::Authenticator`](https://github.com/apache/mesos/blob/95faeac356d7918803beeaa77098569383539a16/include/mesos/authentication/http/basic_authenticator_factory.hpp),
>  althrough there the factory is part of Mesos (we had to do such a thing 
> because the interface to be a module was part of libprocess, so we couldn't 
> create a mesos friendly `create()` method there, so we extracted to its own 
> factory class).
>     
>     The reason I went for this pattern was so I didn't have to create an 
> extra factory function in the `LocalAuthorizer` which takes parameters only 
> for  
>     
>     The only alternative I saw to this was to add a constructor which takes 
> `Parameters` to the `LocaAuthorizer` which will be used only for testing 
> purposes, which I didn't want to have. But if you think that path is the 
> right one, please let me know.

LocalAuthorizer::create(const Parameters& parameters) sounds fine to me.


- Vinod


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/44319/#review122418
-----------------------------------------------------------


On March 8, 2016, 4:43 p.m., Alexander Rojas wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44319/
> -----------------------------------------------------------
> 
> (Updated March 8, 2016, 4:43 p.m.)
> 
> 
> Review request for mesos, Adam B, Joerg Schad, and Vinod Kone.
> 
> 
> Bugs: MESOS-2950
>     https://issues.apache.org/jira/browse/MESOS-2950
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Removed initialize method from the authorizer interface.
> 
> 
> Diffs
> -----
> 
>   include/mesos/authorizer/authorizer.hpp 
> ec6c9928c55c3096c7de634f900419abbdd00886 
>   src/authorizer/authorizer.cpp 54278b022118c40d3b976794fd472ce8d8b6a5e2 
>   src/authorizer/local/authorizer.hpp 
> 96baf77709cf721caf46b6c2c096a843c1b5d9c0 
>   src/authorizer/local/authorizer.cpp 
> 4e5c2c2869823ec957735cebfc80dc850d40f9eb 
>   src/examples/test_authorizer_module.cpp 
> 95d77fbff0cdfdb360a8597fbba28404b59d0042 
>   src/local/local.cpp 359fc54d7c4081f536a8de8b1dfcde413d75c9a9 
>   src/master/main.cpp 4263110c9b889984ef74eb94fed629958f2abd79 
>   src/tests/authorization_tests.cpp 2b2297036550412a955ff479f6ec9d7dad8cb0e3 
>   src/tests/cluster.cpp 084fb1ce37a315c561c4587c4761c870f54c8625 
> 
> Diff: https://reviews.apache.org/r/44319/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>

Reply via email to