----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/71269/#review217260 -----------------------------------------------------------
Nice! I was going to give a ship it but I think there's a bug in tryRemove, see below. Also, made a suggestion about using a `[]` operator to simplify things a bit. src/master/allocator/mesos/hierarchical.hpp Lines 150 (patched) <https://reviews.apache.org/r/71269/#comment304542> s/N/n/ src/master/allocator/mesos/hierarchical.hpp Lines 153-154 (patched) <https://reviews.apache.org/r/71269/#comment304534> Rather than say "what", maybe we say "why"? i.e. RoleTree controls all the mutation? src/master/allocator/mesos/hierarchical.hpp Lines 186 (patched) <https://reviews.apache.org/r/71269/#comment304535> Should the comment about when we track should be here instead? src/master/allocator/mesos/hierarchical.hpp Lines 206-209 (patched) <https://reviews.apache.org/r/71269/#comment304536> s/rolePath/role like the others? src/master/allocator/mesos/hierarchical.hpp Lines 219 (patched) <https://reviews.apache.org/r/71269/#comment304537> We generally prefer pointers to non-const references, but especially as a return value? `s/Role&/Role*` src/master/allocator/mesos/hierarchical.hpp Lines 221 (patched) <https://reviews.apache.org/r/71269/#comment304538> s/ path.// src/master/allocator/mesos/hierarchical.hpp Lines 221-225 (patched) <https://reviews.apache.org/r/71269/#comment304539> Maybe something about how this must be called internally whenever a role becomes empty? Otherwise the tree invariants will break. src/master/allocator/mesos/hierarchical.hpp Lines 227 (patched) <https://reviews.apache.org/r/71269/#comment304540> basename? End with a period src/master/allocator/mesos/hierarchical.hpp Lines 234 (patched) <https://reviews.apache.org/r/71269/#comment304541> `s/roleMap/roles_` ? src/master/allocator/mesos/hierarchical.cpp Lines 236-243 (patched) <https://reviews.apache.org/r/71269/#comment304543> Avoid the double lookup since this is called a lot more than addChild and removeChild? src/master/allocator/mesos/hierarchical.cpp Lines 253 (patched) <https://reviews.apache.org/r/71269/#comment304544> s/baseName/component/ ? Or.. I `token` as done in tryRemove? src/master/allocator/mesos/hierarchical.cpp Lines 290 (patched) <https://reviews.apache.org/r/71269/#comment304545> Stick with `token` or `component` in both add and tryRemove? src/master/allocator/mesos/hierarchical.cpp Lines 297-300 (patched) <https://reviews.apache.org/r/71269/#comment304546> Whoa.. once you erase from `roleMap` current is pointing to deleted data right? Seems like that has to be done last after no more uses of `current`? ``` CHECK_NOTNULL(current->parent)->removeChild(current); metrics->removeRole(current->role); Role* parent = CHECK_NOTNULL(current->parent); roleMap.erase(current->role); // current is now deleted. current = parent; ``` src/master/allocator/mesos/hierarchical.cpp Lines 314-316 (patched) <https://reviews.apache.org/r/71269/#comment304547> This makes me think `add` should just be a `[]` style operator: ``` Role* current = (*this)[reservationRole]; ``` i.e. insert if absent, return reference That will avoid all these ternaries, and [] has the right semantics people expect src/master/allocator/mesos/hierarchical.cpp Lines 339-340 (patched) <https://reviews.apache.org/r/71269/#comment304548> Hm.. might be cleaner and more efficient if this variable is declared outside the loop? Ditto for trackReservations? Can the compiler actually realize that it doesn't have to recompute fromScalarResources each time? src/master/allocator/mesos/hierarchical.cpp Lines 344 (patched) <https://reviews.apache.org/r/71269/#comment304551> newline? src/master/allocator/mesos/hierarchical.cpp Lines 352-353 (patched) <https://reviews.apache.org/r/71269/#comment304549> Another nice place for []: ``` Role& r = (*this)[role]; ``` src/master/allocator/mesos/hierarchical.cpp Lines 369-371 (patched) <https://reviews.apache.org/r/71269/#comment304550> The tryRemove if guard is for efficiency, right? Should we just unguard the rest of these, much like untrackReservation doesn't guard it? src/master/allocator/mesos/hierarchical.cpp Lines 377-383 (patched) <https://reviews.apache.org/r/71269/#comment304552> This could look like: ``` (*this)[role].quota_ = quota; if (quota == DEFAULT_QUOTA) { tryRemove(role); } ``` or: ``` (*this)[role].quota_ = quota; tryRemove(role); ``` src/master/allocator/mesos/hierarchical.cpp Line 1400 (original), 1594 (patched) <https://reviews.apache.org/r/71269/#comment304553> newline between the CHECK and this? src/master/allocator/mesos/hierarchical.cpp Lines 1801 (patched) <https://reviews.apache.org/r/71269/#comment304554> Why change this to `rolePath`? We wouldn't need to change the topLevelRole code either? src/master/allocator/mesos/hierarchical.cpp Lines 1618-1619 (original), 1814-1816 (patched) <https://reviews.apache.org/r/71269/#comment304556> We already have `role` as the key, should be the same as `r.role`? Why change it? src/master/allocator/mesos/hierarchical.cpp Line 1666 (original), 1865 (patched) <https://reviews.apache.org/r/71269/#comment304557> Ditto here, just leave it as `role` and avoid disturbing this code? src/master/allocator/mesos/hierarchical.cpp Lines 1677-1685 (original), 1877-1880 (patched) <https://reviews.apache.org/r/71269/#comment304558> Nice! src/master/allocator/mesos/hierarchical.cpp Lines 2493-2507 (original), 2689-2703 (patched) <https://reviews.apache.org/r/71269/#comment304560> Ditto here, s/rolePath/role/ and s/role/r/ - Benjamin Mahler On Aug. 16, 2019, 10:31 p.m., Meng Zhu wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/71269/ > ----------------------------------------------------------- > > (Updated Aug. 16, 2019, 10:31 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 > 8be8dcee04e8fc5b97f730b2f058d14c81678788 > src/master/allocator/mesos/hierarchical.cpp > 580d35a3b71c1f7e851fa0504c6b9f037c05c378 > > > Diff: https://reviews.apache.org/r/71269/diff/6/ > > > 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 > >
