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

Thanks Till! Can you add Meng to any hierachical role related reviews?

I looked through master and agent metrics and agent endpoints and didn't see 
any other places where this should be used.

>From an API perspective, I took at look at what cgroups did for flat vs 
>hierarchical information, look for "total_<counter>" here:

We could probably do something similar here, like:

  "name": "role",
  "weight": 1,
  "quota": 1 cpus, 10GB mem, 100GB disk,
  "allocation": 0
  "total_allocation": 1 cpus, 10GB mem, 100GB disk,
  "frameworks": ..., // frameworks directly subscribed to "role",
  "total_frameworks": ..., // all frameworks subscribed to roles within "role" 

Make sense?

(1) I was also expecting to see the master's `Role::allocatedResources` return 
the correct information for the protobuf rather than logic on top having to do 
the aggregation. Can we have the `Role` struct in the master be able to return 
all the information shown above? (you can tackle the different pieces in 
separate patches)

(2) This also lets us update the 'Frameworks' column of the Roles table to be 
hierarchical via 'total_frameworks', at which point we probably want to call 
that column something like 'Total Frameworks'. I think that's probably what a 
user expects to see in the UI? (E.g. "engineering" contains 10 frameworks, 2 of 
which are in "engineering/frontend" and 8 of which are in 

Also, this would be a great opportunity (once we think we're happy with the API 
change) to publish it to the dev@ list to give folks a chance to give feedback. 
We're trying to start this practice for API changes as part of the API working 

Lines 3476-3477 (patched)

    Any reason you avoided being specific about this being an "allocation" 
tree? (e.g. you wanted to re-use it for other cases?)
    Otherwise, I would suggest calling this:
    // Provides hierarchical accounting of resource allocation.
    // The allocation for a role is the aggregated allocation of
    // its children (and itself since allocations can be made to
    // non-leaf roles).
    // For example:         a (allocation = 1 + 3 = 5)
    //                     / \
    //   (allocation = 1) b   c (allocation = 3)
    // TODO(tillt): This only supports building up the tree and
    // querying allocation. More interface is needed to keep a
    // tree up-to-date as things change.
    class ResourceAllocationTree

Lines 3487 (patched)

    s/resources/allocation/ ?

Lines 3491 (patched)

    Just to be sure, do we disallow double slashes? E.g. "a//b"? If so, then 
tokenize and split provide the same functionality here, and I would be inclined 
to just use split to avoid having to reason about what the implications of 
stripping empty tokens are (e.g. it breaks the lookup logic?)

Lines 3494-3513 (patched)

    The way this is written the root 'Node' does not contain the sum of its 
children's allocations, unlike all other nodes. This doesn't seem like an issue 
since the API does not allow you to query for the root allocation. However, 
might as well make the root 'Node.resources' contain the right information.

Lines 3539-3542 (patched)

    Do we need pointers here? It seems to me the only pointer we need is in 
`nodes`. Then you won't need to do any `delete`s?

Lines 3702-3710 (patched)

    Rather than having to do this in the http code, can we have the Role struct 
directly expose the aggregated information as well? I'd also like the Role 
struct to have quota information, but that's for another patch :)

- Benjamin Mahler

On March 7, 2018, 12:22 a.m., Till Toenshoff wrote:
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65939/
> -----------------------------------------------------------
> (Updated March 7, 2018, 12:22 a.m.)
> Review request for mesos, Benjamin Bannier, Benjamin Mahler, Kapil Arya, and 
> Michael Park.
> Bugs: MESOS-8069
>     https://issues.apache.org/jira/browse/MESOS-8069
> Repository: mesos
> Description
> -------
> When listing role "a", resources allocated to role "a/b" are added to
> those allocated to role "a". These changes affect both, the "/roles"
> endpoint as well as the V1 HTTP Call "GET_ROLES".
> Diffs
> -----
>   src/master/http.cpp 6f692e20b0f82dbe5c676739757b9eeaf4d6d49a 
> Diff: https://reviews.apache.org/r/65939/diff/1/
> Testing
> -------
> make check
> Thanks,
> Till Toenshoff

Reply via email to