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 Wed, 14 Aug 2019 02:58:03 GMT

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


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