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

Seems like the right overall approach, but I've realized that we need to be 
careful about breaking the Allocator module API. Put RoleInfo back and I'll do 
another pass.

src/master/allocator/mesos/allocator.hpp (line 58)

    I don't think we need to break the allocator API for this.
    Sorry for misleading you before, but RoleInfo (and everything in 
include/mesos/master/allocator.hpp) is a part of the allocator module 
interface, potentially in use by community allocator modules. So, we'd have to 
issue an API change proposal/notification if you're actually going to change 
the parameter type. We're running into the opposite issue now with the Isolator 
module, where we're breaking the API to introduce a protobuf 
IsolatorRecoveryInfo parameter just to hold work_dir. Once we have the 
protobuf, we can then add new optional parameters without breaking the API 

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

    I'm not sure how badly we need to clean up this map. What happens if we 
leave a role in the activeRoles list even after a framework unregisters? Will 
it make the roleSorter noticeably slower?

src/master/master.hpp (lines 1332 - 1333)

    Would an `Option<hashset<string>>` make the `if(explicitRoles.isNone())` 
check more readable than `if(validRoles.empty())`?


    Seems strange that you wouldn't replace this with a `Role(string name, 
double weight)` constructor. You complain about Role not knowing its 
name/weight elsewhere.

src/master/master.cpp (lines 552 - 556)

    We don't this kind of `*`-boxing for deprecation warnings.
    We also don't usually get excited enough to use exclamation marks.

src/master/master.cpp (line 5534)

    How about a CHECK << "message" explaining what's failing?

src/master/master.cpp (lines 5534 - 5538)

    For how many times you say `framework->info.role()`, you should probably 
assign it to a local variable.

src/master/master.cpp (lines 5827 - 5830)

    I guess with Role.RoleInfo or Role.Weights, you wouldn't delete the Role if 
it had a non-default weight.

src/master/quota_handler.cpp (line 303)

    s/permitted/on the role whitelist, if it exists/
    "permitted" sounds too much like ACL permissions to me.

- Adam B

On Dec. 8, 2015, 9:54 p.m., Neil Conway wrote:
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41075/
> -----------------------------------------------------------
> (Updated Dec. 8, 2015, 9:54 p.m.)
> Review request for mesos, Adam B, Alexander Rukletsov, Benjamin Hindman, Greg 
> Mann, and Yong Qiao 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 5d33138d60e684b23f07e1781de7991209d3e161 
>   src/master/master.hpp 4683fa542a740f9a0b80fff7fbe0e63ec66266f2 
>   src/master/master.cpp 7acdc0a5d23a606eac2f37f4b7dd021c5a4fceb7 
>   src/master/quota_handler.cpp b209da42ace752953686eeda9577007a33556d5d 
>   src/tests/allocator.hpp c7670525765491fe931a4ee38446fa7e9d79af42 
>   src/tests/hierarchical_allocator_tests.cpp 
> fb214a829a57529d3f5c49730ae9733f53e622ca 
> 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