This is an automatically generated e-mail. To reply, visit:

Good progress. I'm more confident now in the removal of RoleInfo in favor of a 
weights map.
- I'm liking the idea of naming this a role "whitelist" rather than 
- We need to decide if /roles should care about quota. I vote "not right now"
- Are you planning to "update documentation" and "add tests" in a separate 

src/master/allocator/mesos/hierarchical.hpp (lines 367 - 368)

    "Roles are removed from this map..." and more importantly is removed from 
the roleSorter and has its frameworkSorter deleted.

src/master/allocator/mesos/hierarchical.cpp (lines 997 - 1012)

    If the allocator's roleSorter doesn't know about the role (i.e. no 
frameworks are registered with that role), do we still want to make an explicit 
`allocate()` call on a SetQuota request for that role? Will setting quota on an 
unregistered role have any impact on the fair shares of other frameworks?


    If there are no registered frameworks in any role, do we want to early-exit 
from the allocate() call?

src/master/http.cpp (line 316)

    I'm in agreement with Yong. This form of `model(role, name, weight)` isn't 
used anywhere, and is confusing alongside `modelRole(roleName, weight, 
Option<role>)`. Remove it and rename the other one to fit the `model(...)` 

src/master/http.cpp (lines 1601 - 1602)

    Why do you list unregistered roles with quota configured, if you don't 
model/display their quota?
    If you think quota (and roles configured for quota) should show up in 
`/roles`, file a JIRA and we'll do it all right in one pass.

src/master/http.cpp (lines 1616 - 1619)

    Would it be appropriate to make this an `Option<double> weight` and set the 
default display value in the `model()` function rather than in the request 

src/master/master.hpp (lines 1336 - 1338)

    I'm hesitant on this `validRoles`/`validRole()` naming, especially as I 
review another patch about valid role names based on special characters being 
    How about `explicitRoles`, or `roleWhitelist`? If there is no explicit role 
whitelist, then any role is allowed, but in the presence of a whitelist, any 
new role has to be on the list.

src/master/master.cpp (lines 553 - 554)

    Why do you need the '\n' in your LOG message? Seems like an unnecessarily 
short wrap.

src/master/master.cpp (line 2658)


src/master/quota_handler.cpp (lines 181 - 192)

    Unnecessary change. `rescindOffers()` is only called from 
`Master::QuotaHandler::set()`, which will return BadRequest `if 
(!master->roles.contains(role))`, so this is indeed validated earlier.
    Besides, if `master->roles` didn't container `role`, wouldn't you want to 
exit out of the whole function rather than just skip that loop?

- Adam B

On Dec. 14, 2015, 3:16 p.m., Neil Conway wrote:
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41075/
> -----------------------------------------------------------
> (Updated Dec. 14, 2015, 3:16 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 37fbcb93074fe189133161afaa28046dc2ad0731 
>   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:
> * Update documentation
> * Add tests for allocation behavior for weights + implicit roles
> * Add 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