> On Dec. 15, 2015, 1:56 a.m., Adam B wrote:
> > src/master/allocator/mesos/hierarchical.cpp, lines 1016-1031
> > <https://reviews.apache.org/r/41075/diff/6/?file=1163905#file1163905line1016>
> >
> >     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?
> Neil Conway wrote:
>     It is possible for `roleSorter` to not know about the role but for 
> `quotaRoleSorter` to know about it -- i.e., if the role has a configured 
> quota but no active frameworks. In general, I can imagine circumstances in 
> which we would still want to call `allocate()` here (e.g., if we end up 
> revoking/rescinding offers to non-quota frameworks to preserve resources 
> needed by a quota'd role). Perhaps AlexR can comment?
> Alexander Rukletsov wrote:
>     Once quota is set for a role, the next `allocate()` call will lay away 
> resources. I think Adam means that we do not necessarily need to allocate 
> straight away because there are no consumers (no active frameworks in the 
> quota'ed role) of these resources. Hence though quota on an unregistered role 
> (I assume it means a role without any frameworks) does impact other 
> frameworks, we should not necessarily rush with `allocate()`.
>     However, does it make sense to introduce a condition here? What is the 
> advantage? One extra `allocate()` call should not be a big deal, since set 
> quota operation won't happen that often. So even Adam has the point, I would 
> leave the code as is for readability.

"Adam means that we do not necessarily need to allocate straight away because 
there are no consumers"
That is indeed what I meant.
"One extra allocate() call should not be a big deal... I would leave the code 
as is for readability."
SGTM. Dropping the issue.

> On Dec. 15, 2015, 1:56 a.m., Adam B wrote:
> > src/master/http.cpp, line 316
> > <https://reviews.apache.org/r/41075/diff/6/?file=1163906#file1163906line316>
> >
> >     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(...)` pattern.
> Neil Conway wrote:
>     Whoops, this version of `model(role, name, weight)` was included 
> accidentally (it is from an old version of the patch). Will remove.
>     I'm not sure that changing `modelRole()` to use the `model(...)` naming 
> pattern is the right thing to do: the pattern is that `model(X)` returns a 
> JSON object modeled after `X`. In this case, the parameters will be `string 
> name, double weight, Option<Role*>`, so (a) it doesn't quite fit (b) from the 
> function name + arguments, it is a little unclear that `model(string name, 
> double weight, Option<Role*>)` is supposed to do.

Although precedent exists, I would agree that it is also unclear what the 
following existing `model(...)` call is supposed to do:
JSON::Object model(
    const TaskInfo& task,
    const FrameworkID& frameworkId,
    const TaskState& state,
    const std::vector<TaskStatus>& statuses);

> On Dec. 15, 2015, 1:56 a.m., Adam B wrote:
> > src/master/http.cpp, lines 1601-1602
> > <https://reviews.apache.org/r/41075/diff/6/?file=1163906#file1163906line1601>
> >
> >     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.
> Neil Conway wrote:
>     This was per discussion with AlexR. The idea is that, previously, 
> "/roles" showed all the "potentially interesting" roles: by definition, it 
> will include any role that has a non-default weight or non-default quota. 
> With implicit roles, we want to show the same set of roles that have 
> configured properties.
>     Whether we show a role with configured quota is separate from whether we 
> show quota information about any of the roles. For the latter, not sure if we 
> want that information in "/roles", quota-related endpoints, or both. In any 
> case, seems fine to defer this part of it.
> Alexander Rukletsov wrote:
>     That's correct. Let me repeat my argument one more time: once a quota 
> becomes "whitelisted" or, I prefer, "visible", we should show it in "/roles" 
> to make operator's life easier. If an operator sets a quota for a role, they 
> should see this role in "/roles", otherwise it becomes very tedious to track 
> all "whitelisted" roles and potential typos.

Fair enough. Dropping this issue.
Just make sure that ROLES_HELP accurately explains this. I'll open a separate 
issue for that.

- Adam

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

On Dec. 16, 2015, 1:08 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, 1:08 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