----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/71269/#review217222 -----------------------------------------------------------
I think the biggest thing to consider here is that the current interface still allows the caller to easily break the tracking invariants: the caller has to ensure that it always calls tryRemove whenever making a modification to Role that could make it "empty". Much like the reservation tracking, we could resolve this by having functions in the tree class for: - update quota - update weight - track / untrack framework id That will make the Role read-only for a caller, and enforce the role tracking invariants internally without the caller having to care about it. See comment below. src/master/allocator/mesos/hierarchical.hpp Line 112 (original), 113-119 (patched) <https://reviews.apache.org/r/71269/#comment304509> an extra 'or' and should be 'subscribed' also, since we have a list, would be clearer to bullet it and indent it: ``` // We track a role when it has: // // * a non-default weight, or // * a non-default quota, or // * frameworks subscribed to it, or // * reservations, or // * descendent roles meeting any of the above conditions. ``` src/master/allocator/mesos/hierarchical.hpp Line 119 (original), 129 (patched) <https://reviews.apache.org/r/71269/#comment304528> s/getR/r/ src/master/allocator/mesos/hierarchical.hpp Lines 145-146 (patched) <https://reviews.apache.org/r/71269/#comment304508> Align the comments or don't? ``` const std::string role; // E.g. "a/b/c" const std::string baseName; // E.g. "c" ``` or: ``` const std::string role; // E.g. "a/b/c" const std::string baseName; // E.g. "c" ``` src/master/allocator/mesos/hierarchical.hpp Lines 183-193 (patched) <https://reviews.apache.org/r/71269/#comment304510> Ok, now that I understand that you can reason about this tree as having leaves that are {framework id, non default weight, non default quota, reservations}, it's clearer to me. This example doesn't really help with that, so I think we can do without it. src/master/allocator/mesos/hierarchical.hpp Lines 203-204 (patched) <https://reviews.apache.org/r/71269/#comment304511> s/getAllRoles/roles/ src/master/allocator/mesos/hierarchical.hpp Lines 211-214 (patched) <https://reviews.apache.org/r/71269/#comment304526> > // Try to remove the role associated with the role. whoops? > The role and its ancestors will be removed if they become "empty". This seems a little inaccurate given it's only the ancestors that can "become" empty during this call, whereas the role must "be" empty to be removed? This is a significant break in the interface where the caller has to be very careful to call tryRemove whenever potentially making an Role empty. It exists because we've allowed mutation on Role. We could make it automatic if we added additional mutation functions in the tree itself, just like we did for trackReservations/untrackReservations: - track / untrack framework id - update quota - update weight With these, we also won't need the `add(role)` function, the Role tracking will be fully managed by the tree interface and the caller can't screw it up by adding an empty Role or forgetting to tryRemove an empty Role. Re-iterated this in the top-level comment. src/master/allocator/mesos/hierarchical.hpp Lines 217-224 (patched) <https://reviews.apache.org/r/71269/#comment304527> Just my 2 cents, it would have been better to do the `hashmap<std::string, Resources> -> Resources` change in front of this patch or in this patch if that's not possible, so that there's 1 less thing the reviewer has to hold in their head. src/master/allocator/mesos/hierarchical.cpp Lines 1401-1403 (original), 1567-1577 (patched) <https://reviews.apache.org/r/71269/#comment304529> Yikes.. all this logic would become: ``` roleTree.updateQuota(role, quota); ``` If we have the tree take care of the lifecycle management. src/master/allocator/mesos/hierarchical.cpp Line 1419 (original), 1595-1606 (patched) <https://reviews.apache.org/r/71269/#comment304530> Ditto here. - Benjamin Mahler On Aug. 16, 2019, 3:36 a.m., Meng Zhu wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/71269/ > ----------------------------------------------------------- > > (Updated Aug. 16, 2019, 3:36 a.m.) > > > Review request for mesos, Andrei Sekretenko and Benjamin Mahler. > > > Bugs: MESOS-9917 > https://issues.apache.org/jira/browse/MESOS-9917 > > > Repository: mesos > > > Description > ------- > > The role concept in Mesos fits into a tree structure naturally. > However, the role state in the allocator are currenstored > in a hashmap. This is less efficient and harder to use and reason. > > This patch introduced a `RoleTree` structure in the allocator > and organizes all the roles in to a tree. This should simplify > the code logic and opens further refactor and optimization > opportunities. > > In addition, the master code also lacks a proper tree structure > for tracking roles. We should leverage the same role tree code > here to simplify that as well. > > > Diffs > ----- > > src/master/allocator/mesos/hierarchical.hpp > 8be8dcee04e8fc5b97f730b2f058d14c81678788 > src/master/allocator/mesos/hierarchical.cpp > 580d35a3b71c1f7e851fa0504c6b9f037c05c378 > > > Diff: https://reviews.apache.org/r/71269/diff/4/ > > > Testing > ------- > > make check > > Benchmaring using optimized build shows no noticable performance change. > > Benchmark: `BENCHMARK_HierarchicalAllocator_WithQuotaParam.LargeAndSmallQuota` > > ## Before: > > Added 30 agents in 1.252621ms > Added 30 frameworks in 7.747202ms > Benchmark setup: 30 agents, 30 roles, 30 frameworks, with drf sorter > Made 36 allocations in 12.791427ms > > Added 300 agents in 9.765295ms > Added 300 frameworks in 273.978325ms > Benchmark setup: 300 agents, 300 roles, 300 frameworks, with drf sorter > Made 350 allocations in 424.603736ms > > Added 3000 agents in 79.646516ms > Added 3000 frameworks in 19.17449514secs > Benchmark setup: 3000 agents, 3000 roles, 3000 frameworks, with drf sorter > Made 3500 allocations in 31.687207066secs > > ## After: > > Added 30 agents in 1.376619ms > Added 30 frameworks in 7.647487ms > Benchmark setup: 30 agents, 30 roles, 30 frameworks, with drf sorter > Made 36 allocations in 11.638116ms > > Added 300 agents in 9.506101ms > Added 300 frameworks in 263.008198ms > Benchmark setup: 300 agents, 300 roles, 300 frameworks, with drf sorter > Made 350 allocations in 418.962396ms > > Added 3000 agents in 81.553527ms > Added 3000 frameworks in 19.201708305secs > Benchmark setup: 3000 agents, 3000 roles, 3000 frameworks, with drf sorter > Made 3500 allocations in 31.583417136secs > > > Thanks, > > Meng Zhu > >
