mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Benjamin Mahler <bmah...@apache.org>
Subject Re: Review Request 71269: Added a role tree class in the allocator.
Date Thu, 15 Aug 2019 16:44:58 GMT


> 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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message