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


Fix it, then Ship it!




Looks great! Just one issue with variable naming (one underscore doesn't bother 
me much, but two does)


docs/configuration.md (lines 937 - 939)
<https://reviews.apache.org/r/45922/#comment193654>

    Unnecessary here; only necessary for the --acls flag.



src/slave/main.cpp (line 292)
<https://reviews.apache.org/r/45922/#comment193655>

    You can leave out the quotes. I just wanted the hyphenation



src/tests/cluster.cpp (line 392)
<https://reviews.apache.org/r/45922/#comment193656>

    Double underscores for variable names are discouraged (usually only 
acceptable for continuation function names).
    Maybe the original parameter is `providedAuthorizer`, and `authorizer__` 
becomes `newAuthorizer`, then you could have:
    `authorizer = providedAuthorizer;`
    `if(...) newAuthorizer = Authorizer::create();`
    `if(newAuthorizer.isSome()) authorizer = newAuthorizer;`
    Or maybe you've got better names, but you get the idea.



src/slave/main.cpp (lines 291 - 293)
<https://reviews.apache.org/r/45922/#comment193668>

    Let's move this comment into the `if (authorizerName != 
slave::DEFAULT_AUTHORIZER)` block, since that's the condition it applies to. 
Then you can even simplify it down to "NOTE: The contents of --acls are ignored 
for custom authorizers."



src/tests/cluster.cpp (lines 389 - 391)
<https://reviews.apache.org/r/45922/#comment193669>

    Simplify and move to before `Authorizer::create(authorizerName);`


- Adam B


On April 21, 2016, 7:48 a.m., Jan Schlicht wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45922/
> -----------------------------------------------------------
> 
> (Updated April 21, 2016, 7:48 a.m.)
> 
> 
> Review request for mesos, Adam B and Alexander Rojas.
> 
> 
> Bugs: MESOS-5142
>     https://issues.apache.org/jira/browse/MESOS-5142
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> See summary.
> 
> 
> Diffs
> -----
> 
>   docs/configuration.md 86ba66ac62295ca148524bcb2e57fee560ac4ac5 
>   src/local/local.cpp 7de8a2423185e49dfa849d069938d3243b4f4956 
>   src/slave/constants.hpp 9978c11fec40055dd42f19c20cd3e9fef4e78cea 
>   src/slave/flags.hpp ee520acc459564fe68272950948fc80c5e24513a 
>   src/slave/flags.cpp 10d2974bd2b6e79255fc894979607f0d2d00c315 
>   src/slave/main.cpp 38bd00584dd9c6a872398678b2288edeed1cd2a4 
>   src/slave/slave.hpp f78c1b4e4d5378ef7223c6eb3ea45491c30fb4c1 
>   src/slave/slave.cpp d82dec2b10d496065013eb4ad6a35dc054b72553 
>   src/tests/cluster.hpp 96ec52af16776e91200aa755c7847f56e33d71f4 
>   src/tests/cluster.cpp 3e5fdf6b32a0d99d3ca83743386232d38471e34f 
>   src/tests/mesos.hpp e4b63d41d883807ac39846799468b80e88c84e0b 
>   src/tests/mesos.cpp b5937af7713e1ee2af475518b3e968b2defa8beb 
> 
> Diff: https://reviews.apache.org/r/45922/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>

Reply via email to