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


In terms of file structure, I think it makes sense to follow `Authenticator`'s 
structure:

  `include/mesos/authorization`
   |
   +-- `authorization.proto` (ACL definitions)
   +-- `authorization.hpp` (`#include <mesos/authorization/authorization.pb.h>`)
   +-- `authorizer.hpp`

What do you think?


include/mesos/authorizer/authorizer.hpp (lines 29 - 30)
<https://reviews.apache.org/r/36048/#comment149256>

    `+ #include <stout/option.hpp>`



include/mesos/authorizer/authorizer.hpp (lines 35 - 37)
<https://reviews.apache.org/r/36048/#comment149257>

    How about something like:
    
    ```
    An interface used to provide authorization for actions with ACLs. Refer to 
"docs/authorization.md" for the details regarding the authorization mechanism.
    ```
    
    Maybe something with the `@see` thing rather than `Refer to ...`?



include/mesos/authorizer/authorizer.hpp (lines 45 - 47)
<https://reviews.apache.org/r/36048/#comment149259>

    Looks like we call `initialize` even for non-default implementations of 
`Authenticator`, why do we want to call it only for the default implementation 
for `Authorizer`?



include/mesos/authorizer/authorizer.hpp (lines 53 - 54)
<https://reviews.apache.org/r/36048/#comment149260>

    `s/is/if/`
    
    What do we mean by "__could be__ initialized successfully"? I would assume 
if `Nothing` is returned, the authorizer has already been initialized 
successfully?



src/authorizer/authorizer.cpp (lines 209 - 213)
<https://reviews.apache.org/r/36048/#comment149261>

    (1) We don't use `nullptr` yet :( I've just filed [MESOS-3243]( 
https://issues.apache.org/jira/browse/MESOS-3243) to keep track of this, since 
I imagine more and more people will want to use it.
    (2) AFAIK, `new LocalAuthorizer` will never return a nullptr. Am I missing 
something? If not, the comment for this function is also inaccurate.
    
    Sidenote: if we wanted `new` to return a nullptr rather than throwing a 
`std::bad_alloc` in the cases where we run out of memory, we should use `new 
(std::nothrow) LocalAuthorizer` instead. But this isn't the standard practice 
for our codebase.



src/authorizer/authorizer.cpp (line 219)
<https://reviews.apache.org/r/36048/#comment149262>

    `s/nullptr/NULL/` for now, here and below.



src/authorizer/authorizer.cpp (lines 239 - 248)
<https://reviews.apache.org/r/36048/#comment149264>

    Why can't we use `process/once.hpp` here?



src/tests/cluster.hpp (line 356)
<https://reviews.apache.org/r/36048/#comment149265>

    No need to check for `local.get() == nullptr`.


- Michael Park


On Aug. 6, 2015, 11:59 a.m., Alexander Rojas wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36048/
> -----------------------------------------------------------
> 
> (Updated Aug. 6, 2015, 11:59 a.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rukletsov, Bernd Mathiske, Kapil 
> Arya, Jan Schlicht, and Till Toenshoff.
> 
> 
> Bugs: MESOS-2946
>     https://issues.apache.org/jira/browse/MESOS-2946
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Splits and updates the original declaration of the `Authorizer` into its 
> interface and a default implementation, the `LocalAuthorizer`.
> 
> Following the pattern of the modularized `Authenticator`, it generates a 
> default constructor which is required when writing a `TYPED_TEST` in
> a follow up patch. Additionally, an initialize method has been added, needed 
> for passing in the current ACL definitions as provided via
> flags.
> 
> Other changes are just updates to allow for compilation.
> 
> 
> Diffs
> -----
> 
>   include/mesos/authorizer/authorizer.hpp PRE-CREATION 
>   include/mesos/authorizer/authorizer.proto PRE-CREATION 
>   include/mesos/mesos.proto a6748d1cd82238f005c6a49c70d22d095462f1ba 
>   include/mesos/type_utils.hpp f1cb5e279073c5195fc41dada307a10d00c84955 
>   src/Makefile.am 35ebbbd0bd9c9dd059c02ce3dc22c780b929be81 
>   src/authorizer/authorizer.hpp c039d9412780aa199db169b31991bf9f45b07d0f 
>   src/authorizer/authorizer.cpp 21e97e315478a4ca9442af83732665f85eb2f8fc 
>   src/common/parse.hpp 8d7ddd6819dad98cd96d5aaae8fe57caf1ee7098 
>   src/examples/persistent_volume_framework.cpp 
> c6d6ed337bfca91dc146cb31298cabebdbb13509 
>   src/local/local.cpp 1953d84c75a83f4ace944d6243456235d8a193ff 
>   src/master/flags.hpp f2cd19a6edfaa4e5bb31f024ef8d5beda32fbc2f 
>   src/master/http.cpp 76e70801925041f08bc94f0ca18c86f1a573b2b3 
>   src/master/main.cpp e05a472b86170eb26df26aaa4b65437fcdd413ce 
>   src/master/master.hpp 30a2550c606ca528ec5b69fc9efedd698d67c5f2 
>   src/master/master.cpp 50b98248463fc4cd48962890c14c7ad64f2b6f43 
>   src/tests/authorization_tests.cpp 99bb06c1ee73a90abaeeabb742e45aa188c21a87 
>   src/tests/cluster.hpp ba17c0c74a9dc36c595c4ad77fe68be94c5c7c0b 
>   src/tests/mesos.hpp 20418d4fbd2f4ae35ee0c707472cbf37125883b0 
>   src/tests/mesos.cpp a2a469e2a581dc6c566dafd4acd2a95c0238399f 
> 
> Diff: https://reviews.apache.org/r/36048/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>

Reply via email to