----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/71269/#review217192 -----------------------------------------------------------
Nice to see this, and looking forward to it tracking quota consumption too! :P I did a pass on the interface first, suggestions below: src/master/allocator/mesos/hierarchical.hpp Lines 113-117 (patched) <https://reviews.apache.org/r/71269/#comment304480> a bulleted list would be more clear than the long setence with "and/or" cases? src/master/allocator/mesos/hierarchical.hpp Lines 119 (patched) <https://reviews.apache.org/r/71269/#comment304481> Rather than '"Empty" roles are removed immediately. See `Role::isEmpty()`.', I think just referring to not meeting any of the cases in the list would be more clear, e.g. We track roles when: - The role has non-default weight, or - The role has non-default quota, or - ... - or it has descendent roles meeting any of the above conditions Any roles that do not meet these conditions are not tracked in the role tree. src/master/allocator/mesos/hierarchical.hpp Lines 124 (patched) <https://reviews.apache.org/r/71269/#comment304482> Passing `name` here seems prone to confusion vs passing `path`. (I had to go look at the implementation to figure out if `name` was the last part of the role or the entire role). Why not just pass `path` and avoid the confusion? Is there any utility in passing the last component of the "/" split of the role? Personally, I think s/path/role is just as clear, and avoids having to think about the "/" splitting: ``` Role(const std::string& role, Role* parent); ``` Alternatively, `basename` seems like the clear expression of the last component (borrowed from unix terminology). src/master/allocator/mesos/hierarchical.hpp Line 112 (original), 128 (patched) <https://reviews.apache.org/r/71269/#comment304483> Hm.. why can't `path` be const to avoid the getter? If we absolutely need the getter, `path()` seems more idiomatic to our code than `getPath()`. src/master/allocator/mesos/hierarchical.hpp Lines 130 (patched) <https://reviews.apache.org/r/71269/#comment304484> s/getR/r/ src/master/allocator/mesos/hierarchical.hpp Line 119 (original), 144 (patched) <https://reviews.apache.org/r/71269/#comment304486> s/getC/c/ src/master/allocator/mesos/hierarchical.hpp Lines 152-155 (patched) <https://reviews.apache.org/r/71269/#comment304485> Per above, `basename` seems less ambiguous. It also seems clearer and warrants less of a long comment about it, if next to each other: ``` const std::string role; // E.g. "a/b/c" const std::string basename; // E.g. "c" ``` src/master/allocator/mesos/hierarchical.hpp Lines 190 (patched) <https://reviews.apache.org/r/71269/#comment304479> Seems like we can live without this line src/master/allocator/mesos/hierarchical.hpp Lines 198-199 (patched) <https://reviews.apache.org/r/71269/#comment304487> The comment isn't adding value here IMO s/getR/r/ Actually, couldn't this be just touching the member variable? It seems strange that you can only touch the root in a const way but can touch the Roles from `get()` in a non-const way? src/master/allocator/mesos/hierarchical.hpp Lines 201 (patched) <https://reviews.apache.org/r/71269/#comment304488> Typo in exists, but I don't think this comment is needed. Unless there's some subtlety to know about, it seems very clear from the signature. src/master/allocator/mesos/hierarchical.hpp Lines 202 (patched) <https://reviews.apache.org/r/71269/#comment304489> Alternatively, s/path/role/ would be perfectly clear for all these functions in the RoleTree. It doesn't seem like `path` is offering an important distinction in this class' interface? src/master/allocator/mesos/hierarchical.hpp Lines 207-210 (patched) <https://reviews.apache.org/r/71269/#comment304490> There seems to be a bit of subtlety here that is missed in the comment? If I add "a/b", the ancestor role "a" along the path will be created. Then, if I add "a", the comment suggests that it already exists which is a problem. src/master/allocator/mesos/hierarchical.hpp Lines 212-215 (patched) <https://reviews.apache.org/r/71269/#comment304491> Ditto here, it seems at the overall explanation of the RoleTree abstraction we need to explain the "." subtlety, along with an example role tree to make it clear. src/master/allocator/mesos/hierarchical.hpp Lines 218-224 (patched) <https://reviews.apache.org/r/71269/#comment304476> It's quite strange that these take maps with the role (seems like an artifact of the way the allocator managed reservations previously?), the following interface seems more expected: ``` void trackReservations(const Resources& reservations); void untrackReservations(const Resources& reservations); ``` The implementation would examine the resources to know which roles the reservations are for. The caller can call it multiple times for different roles if they like, or pass in Resources that have a mix of reservations. As it stands, it seems pretty brittle and unnecessary to assume that the Resources objects obey each role key. src/master/allocator/mesos/hierarchical.hpp Lines 223 (patched) <https://reviews.apache.org/r/71269/#comment304492> no newline needed? src/master/allocator/mesos/hierarchical.hpp Lines 231 (patched) <https://reviews.apache.org/r/71269/#comment304478> This can be `hashmap<std::string, Role>` which would avoid the potential leak of forgetting `delete`, and avoid the extra memory allocation. src/master/allocator/mesos/hierarchical.cpp Lines 2658-2664 (original), 2799-2805 (patched) <https://reviews.apache.org/r/71269/#comment304477> Per the above comment, maybe this becomes: ``` Resources oldReservations = oldTotal.reserved(); Resources newReservations = total.reserved(); if (oldReservations != newReservations) { roleTree.untrackReservations(oldReservations); roleTree.trackReservations(newReservations); } ``` Or more simply: ``` if (oldTotal.reserved() != total.reserved()) { roleTree.untrackReservations(oldTotal.reserved()); roleTree.trackReservations(total.reserved()); } ``` Or don't even bother equality checking! ``` roleTree.untrackReservations(oldTotal.reserved()); roleTree.trackReservations(total.reserved()); ``` - Benjamin Mahler On Aug. 13, 2019, 10:05 p.m., Meng Zhu wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/71269/ > ----------------------------------------------------------- > > (Updated Aug. 13, 2019, 10:05 p.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 > dee5bafb7d0e56f382b647c649a702cd64e6500a > src/master/allocator/mesos/hierarchical.cpp > 87b03d3a0a2bc9113c6c488ddfc1437651bf58b7 > > > Diff: https://reviews.apache.org/r/71269/diff/2/ > > > 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 > >
