----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/65939/#review198755 -----------------------------------------------------------
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: https://www.kernel.org/doc/Documentation/cgroup-v1/memory.txt 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" tree. } ``` 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 "engineering/backend"). 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 group. src/master/http.cpp Lines 3476-3477 (patched) <https://reviews.apache.org/r/65939/#comment278980> 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 ``` src/master/http.cpp Lines 3487 (patched) <https://reviews.apache.org/r/65939/#comment278986> s/resources/allocation/ ? src/master/http.cpp Lines 3491 (patched) <https://reviews.apache.org/r/65939/#comment278984> 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?) src/master/http.cpp Lines 3494-3513 (patched) <https://reviews.apache.org/r/65939/#comment278988> 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. src/master/http.cpp Lines 3539-3542 (patched) <https://reviews.apache.org/r/65939/#comment278982> 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? src/master/http.cpp Lines 3702-3710 (patched) <https://reviews.apache.org/r/65939/#comment279080> 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 > >
