> On Aug. 14, 2019, 2:58 a.m., Benjamin Mahler wrote: > > src/master/allocator/mesos/hierarchical.hpp > > Lines 212-215 (patched) > > <https://reviews.apache.org/r/71269/diff/2/?file=2160787#file2160787line212> > > > > 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. > > Meng Zhu wrote: > I am not sure what you mean by the "." subtlety. Unlike sorter, there is > no need for any virtual node ".". I think the tree fits in the normal > expectation. But drew an example nevertheless.
Oh, somehow I didn't realize that this tree is not using the "." approach of the sorter's tree. My first impression of that is that we'd have a problem with role lifecycle, but I guess it's ok because the following invariant holds: * If we remove a role implicitly, then it's the case that the caller will not try to remove it. And now I'm realizing that we don't need "." because we model that using framework IDs (or other non-default configs) as the "leaves" (effectively). > On Aug. 14, 2019, 2:58 a.m., Benjamin Mahler wrote: > > src/master/allocator/mesos/hierarchical.hpp > > Lines 231 (patched) > > <https://reviews.apache.org/r/71269/diff/2/?file=2160787#file2160787line231> > > > > This can be `hashmap<std::string, Role>` which would avoid the > > potential leak of forgetting `delete`, and avoid the extra memory > > allocation. > > Meng Zhu wrote: > hmm, would prefer keep the map just as a simple lookup table. Feels > somewhat convoluted if also using it to manage the life cycle. > The simple destructors should be enough to guard against any leaks. > Also, the "get" call would need to return a coy of `Role` instead of a > pointer, not quite what we want. > hmm, would prefer keep the map just as a simple lookup table. Feels somewhat > convoluted if also using it to manage the life cycle. The map is already managing lifecycle? No `Role*` should exist outside the map, right? If we remove a `Role*` from the map, it needs to be `delete`ed right? > Also, the "get" call would need to return a coy of Role instead of a pointer, > not quite what we want. Why would you need to copy? You can just return a pointer to the value from the map, that's no problem? - Benjamin ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/71269/#review217192 ----------------------------------------------------------- On Aug. 14, 2019, 9:45 p.m., Meng Zhu wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/71269/ > ----------------------------------------------------------- > > (Updated Aug. 14, 2019, 9:45 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/3/ > > > 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 > >
