mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Meng Zhu <m...@mesosphere.io>
Subject Re: Review Request 71269: Added a role tree class in the allocator.
Date Wed, 14 Aug 2019 21:45:48 GMT


> On Aug. 13, 2019, 7:58 p.m., Benjamin Mahler wrote:
> > src/master/allocator/mesos/hierarchical.hpp
> > Lines 198-199 (patched)
> > <https://reviews.apache.org/r/71269/diff/2/?file=2160787#file2160787line198>
> >
> >     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?

yeah, I think we can just return a non-const root now. The Role class after some refactoring,
now is pretty const already.


> On Aug. 13, 2019, 7:58 p.m., Benjamin Mahler wrote:
> > src/master/allocator/mesos/hierarchical.hpp
> > Lines 207-210 (patched)
> > <https://reviews.apache.org/r/71269/diff/2/?file=2160787#file2160787line207>
> >
> >     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.

Updated the comment to "The given role must not already exist.".


> On Aug. 13, 2019, 7:58 p.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.

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.


> On Aug. 13, 2019, 7:58 p.m., Benjamin Mahler wrote:
> > src/master/allocator/mesos/hierarchical.hpp
> > Lines 218-224 (patched)
> > <https://reviews.apache.org/r/71269/diff/2/?file=2160787#file2160787line218>
> >
> >     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.

Sg. posted a separate patch: r/71291


> On Aug. 13, 2019, 7:58 p.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.

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.


> On Aug. 13, 2019, 7:58 p.m., Benjamin Mahler wrote:
> > src/master/allocator/mesos/hierarchical.cpp
> > Lines 2658-2664 (original), 2799-2805 (patched)
> > <https://reviews.apache.org/r/71269/diff/2/?file=2160788#file2160788line2877>
> >
> >     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());
> >     ```

Sg. r/71291


- Meng


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71269/#review217192
-----------------------------------------------------------


On Aug. 14, 2019, 2: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, 2: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