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

Ship it!


Looks great! Only a couple of nits. Update the ROLES_HELP, and I'll commit it!


src/master/allocator/mesos/hierarchical.cpp (line 1030)
<https://reviews.apache.org/r/41075/#comment170803>

    Do we need to `CHECK(quotaRoleSorter.contains(role))`?



src/master/http.cpp (lines 1517 - 1518)
<https://reviews.apache.org/r/41075/#comment170777>

    Please update this to explain that the endpoint shows the weight, total 
reserved resources, and registered frameworks for each role that is 
whitelisted, registered, or has a non-default weight or quota.
    The current TLDR/description does not tell me what to expect.



src/master/http.cpp (line 1524)
<https://reviews.apache.org/r/41075/#comment170791>

    Please wrap each parameter onto a separate line, since the entire function 
prototype doesn't fit on one line.



src/master/http.cpp (line 1539)
<https://reviews.apache.org/r/41075/#comment170792>

    I often get confused about our official 'underscore' naming policy, but I 
think this should be `role = _role.get();`
    "We prepend constructor and function arguments with a leading underscore to 
avoid ambiguity and / or shadowing"
    "Some trailing underscores are used to distinguish between similar 
variables in the same scope (think prime symbols), but this should be avoided 
as much as possible, including removing existing instances in the code base."


- Adam B


On Dec. 16, 2015, 2:43 p.m., Neil Conway wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41075/
> -----------------------------------------------------------
> 
> (Updated Dec. 16, 2015, 2:43 p.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rukletsov, Benjamin Hindman, Greg 
> Mann, and Yongqiao Wang.
> 
> 
> Bugs: MESOS-4085
>     https://issues.apache.org/jira/browse/MESOS-4085
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Changed the behavior of the master when the `--roles` flag is NOT
> specified. Previously, this would allow only the `*` role to be used. Now,
> omitting `--roles` means that any role can be used. This is called "implicit
> roles". Configuring which principals can perform operations as which roles
> should be done using ACLs in the authorization system.
> 
> Note that this changes the behavior of the system when `--roles` is not
> specified. This is likely acceptable: if the operator didn't specify `--roles`
> in prior versions of Mesos, they were likely not using roles or authorization 
> at
> that time.
> 
> Another minor behavioral change is that the "/roles" endpoint will now only
> return results for currently "active" roles (those with one or more registered
> frameworks).
> 
> The `--roles` flag is now considered deprecated and will be removed in a 
> future
> version of Mesos.
> 
> 
> Diffs
> -----
> 
>   include/mesos/master/allocator.hpp 619ef01c3a7d640560653cfc1838dd09046d1da0 
>   include/mesos/master/allocator.proto 
> 702f56f56c3b1331613cecf26522986f6b572f8c 
>   src/master/allocator/mesos/allocator.hpp 
> 97ee80726ad155917811265a983258b0165d3451 
>   src/master/allocator/mesos/hierarchical.hpp 
> 99c742906874c30c39c159e58a65277ade3c07fd 
>   src/master/allocator/mesos/hierarchical.cpp 
> 5da825a1d578a9ee40b4985378fddb3c5fb3b416 
>   src/master/http.cpp 8dfd67f7d51b8395953d6beb77b2d71bc538eacd 
>   src/master/master.hpp 9aa548aa6e159046c94e4ec96f631ea8b3bfd5d8 
>   src/master/master.cpp 0d1482279c68f2a4a27dabaf28774769a5d515c4 
>   src/master/quota_handler.cpp 11167879b2480d9c8dd6398ca39c479089ec2272 
>   src/tests/allocator.hpp c7670525765491fe931a4ee38446fa7e9d79af42 
>   src/tests/hierarchical_allocator_tests.cpp 
> e239b4746494fcc2b362a83afb634a2ce5e25f9b 
>   src/tests/master_quota_tests.cpp 0473869783a714766ed26fff61d7f8c56342df74 
> 
> Diff: https://reviews.apache.org/r/41075/diff/
> 
> 
> Testing
> -------
> 
> "make check" on OSX 10.10 and Ubuntu 15.10; `--gtest_repeat=1000` for the 
> more likely role-related tests.
> 
> TODOs:
> 
> * Add tests for allocation behavior for weights + implicit roles
> * More tests for quota + implicit roles?
> 
> Notes:
> 
> * There's two places where we use manual `new`/`delete` where a `unique_ptr` 
> would probably be nicer. I'm inclined to leave this as-is for now though 
> (making use of unique_ptr is a broader issue).
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Reply via email to