> On July 3, 2015, 10:29 a.m., Till Toenshoff wrote:
> > src/authorizer/authorizer.cpp, lines 17-28
> > <https://reviews.apache.org/r/36049/diff/5/?file=998603#file998603line17>
> >
> >     Great update! But it seems the comment is not applying, no?
> >     
> >     The built-in authorizer factory `Try<Authorizer*> 
> > LocalAuthorizer::create()` does not seem to test for `nullptr`. You seem to 
> > do those checks in master.cpp and local.cpp instead. Or am I missing the 
> > point here?

`LocalAuthorizer::create()` now returns an error if new fails, i.e. returns a 
null pointer. Still not sure if I should remove the check in main.


> On July 3, 2015, 10:29 a.m., Till Toenshoff wrote:
> > src/local/local.cpp, lines 227-228
> > <https://reviews.apache.org/r/36049/diff/5/?file=998604#file998604line227>
> >
> >     You may reach this without an error-message, no?
> >     
> >     How about:
> >     ```
> >     if (create.isError() || create.get() == nullptr) {
> >       EXIT(EXIT_FAILURE) << "Could not create authorizer module '"
> >                          << flags.authorizer << "'"
> >                          << (create.isError() ? ": " + create.error() : "");
> >       ;
> >     }
> >     
> >     ```
> >     
> >     Or actually, that factory can not return a `nullptr` anymore, can it?

Now the comment from the previos issue is true, so create never returns a 
nullptr but instead sets the `Try` to an `Error`.


> On July 3, 2015, 10:29 a.m., Till Toenshoff wrote:
> > src/master/flags.cpp, lines 415-424
> > <https://reviews.apache.org/r/36049/diff/5/?file=998608#file998608line415>
> >
> >     We may want to follow the pattern used for the authenticator - as in, 
> > call the flag "authorizers" and prepare the CLI interface for allowing the 
> > activation of multiple, concurrently active authorizers.
> >     
> >     What do you think?

as bespoken.


- Alexander


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


On July 3, 2015, 3:17 p.m., Alexander Rojas wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36049/
> -----------------------------------------------------------
> 
> (Updated July 3, 2015, 3:17 p.m.)
> 
> 
> Review request for mesos and Till Toenshoff.
> 
> 
> Bugs: MESOS-2945
>     https://issues.apache.org/jira/browse/MESOS-2945
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Adds and integrates helper classes needed to support an `Authorizer` module. 
> Also adds a flag to the master, allowing the selection of an `Authorizer` 
> module.
> 
> It also adds a flag which allows to select the module name.
> 
> 
> Diffs
> -----
> 
>   include/mesos/authorizer/authorizer.hpp PRE-CREATION 
>   include/mesos/module/authorizer.hpp PRE-CREATION 
>   src/Makefile.am addb63f615f16ae6b25f745b2e79fd9fc0e27851 
>   src/authorizer/authorizer.cpp PRE-CREATION 
>   src/local/local.cpp 1953d84c75a83f4ace944d6243456235d8a193ff 
>   src/master/constants.hpp 7cec18b7fdfd3b96cde42a30d217c026b2695dce 
>   src/master/constants.cpp fbcae60c43e835f96ec061bd0e9f7961e31fc341 
>   src/master/flags.hpp f2cd19a6edfaa4e5bb31f024ef8d5beda32fbc2f 
>   src/master/flags.cpp 60ac64d98d53f74f904846b27a3833a7c44a9756 
>   src/master/main.cpp 2624b7ea4920a534c98f5dfbf9286c54c50f11a9 
>   src/module/manager.cpp 909ca56eea85d365cb9ebe1b3cce43051cabb670 
> 
> Diff: https://reviews.apache.org/r/36049/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>

Reply via email to