mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Michael Park <mp...@apache.org>
Subject Re: Review Request 57254: Updated DRFSorter to support hierarchical roles.
Date Fri, 10 Mar 2017 01:41:03 GMT

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



bmahler and I went through this together for a bit. We didn't get through everything yet,
but here are a few high-level feedback.

(1) It seems like we could do something similar as the `QuotaTree`, and introduce a tree rather
than a node.
(2) Some naming consistency:
    (a) `find*` functions that return a pointer vs an iterator
    (b) The use of `name` which isn't consistent with `Client::name`, the overall use of `client`,
`label`, `path`, etc.
(3) The lazy generation of the virtual node could be more explicit.
    We don't really have concrete suggestions about this just yet, but maybe for now, more
comments.


src/master/allocator/mesos/hierarchical.hpp
Lines 259-438 (original), 259-431 (patched)
<https://reviews.apache.org/r/57254/#comment240800>

    Could pull this change (no longer storing weights in the allocator) along with the changes
related to this in the sorter out to a separate patch?



src/master/allocator/sorter/drf/metrics.hpp
Lines 44-45 (original), 44-45 (patched)
<https://reviews.apache.org/r/57254/#comment240801>

    (1) Can we pull the changes to `metrics` out to a separate patch? For example, we could
first have a patch that changes the signatures of `Sorter::find` and `Sorter::calculateShare`
(unrelated to the hierarchy changes).
    (2) Do we need this renaming at the API level? If yes, shouldn't this be consistent for
all functions that take `const std::string& client` currently?



src/master/allocator/sorter/drf/sorter.hpp
Lines 68-70 (patched)
<https://reviews.apache.org/r/57254/#comment240808>

    It seems like changing the behavior around the members of the `clients` map, as it simply
gets marked as inactive rather than removed from the map. Is it possible to make this change
before we introduce the role hierarchy?



src/master/allocator/sorter/drf/sorter.hpp
Line 53 (original), 195 (patched)
<https://reviews.apache.org/r/57254/#comment240815>

    `s/'allocations'/'count'/`



src/master/allocator/sorter/drf/sorter.cpp
Line 401 (original), 517 (patched)
<https://reviews.apache.org/r/57254/#comment240817>

    Maybe `sortTree`?



src/master/allocator/sorter/drf/sorter.cpp
Lines 519 (patched)
<https://reviews.apache.org/r/57254/#comment240816>

    How about `sortedChildren`?



src/master/allocator/sorter/drf/sorter.cpp
Line 420 (original), 543 (patched)
<https://reviews.apache.org/r/57254/#comment240819>

    Maybe `listClients`?



src/master/allocator/sorter/drf/sorter.cpp
Lines 545-551 (patched)
<https://reviews.apache.org/r/57254/#comment240821>

    It took a while to figure out how we could end up in the `else` case here. That is, how
could a node not have children, but not be a virtual node (presumably a leaf) I think it's
because of the lazy generation of the virtual node, but that invariant was difficult to deduce
from here. Perhaps we can encapsulate this better, or at least leave comments.
    
    Another thing is, if there's a global invariant that all internal nodes are inactive,
then we don't actually need the `node->children.empty()` check, right? We could potentially
simplify the code leveraging such invariants.


- Michael Park


On March 9, 2017, 8:44 a.m., Neil Conway wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57254/
> -----------------------------------------------------------
> 
> (Updated March 9, 2017, 8:44 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Benjamin Mahler, and Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> This commit replaces the sorter's flat list of clients with a tree of
> client names; this tree represents the hierarchical relationship between
> sorter clients. Each node in the tree contains an (ordered) list of
> pointers to child nodes. The tree might contain nodes that do not
> correspond directly to sorter clients. For example, adding clients "a/b"
> and "c/d" results in the following tree:
> 
> root
>  -> a
>    -> b
>  -> c
>    -> d
> 
> The `sort` member function still only returns one result for each
> (active) client in the sorter. This is implemented by ensuring that each
> sorter client is associated with a leaf node in the tree. Note that it
> is possible for a leaf node to be transformed into an internal node by a
> subsequent insertion; to handle this case, we "implicitly" create an
> extra child node, which maintains the invariant that each client has a
> leaf node. For example, if the client "a/b/x" is added to the tree
> above, the result is:
> 
> root
>  -> a
>    -> b
>      -> .
>      -> x
>  -> c
>    -> d
> 
> The "." leaf node holds the allocation that has been made to the "a/b"
> client itself; the "a/b" node holds the sum of all the allocations that
> have been made to the subtree rooted at "a/b", which also includes
> "a/b/x".
> 
> 
> Diffs
> -----
> 
>   src/master/allocator/mesos/hierarchical.hpp 646f66e67d9c6b8c61fc6e6558a1db976a44c126

>   src/master/allocator/mesos/hierarchical.cpp 0059ccead90f32491591990c791e7fa905a864b7

>   src/master/allocator/sorter/drf/metrics.hpp 61568cb520826ab59d675824b212e0d3deb63764

>   src/master/allocator/sorter/drf/metrics.cpp 15aab32db5ca1a7a14080e9bbb7c65283be3ec20

>   src/master/allocator/sorter/drf/sorter.hpp 76329220e1115c1de7810fb69b943c78c078be59

>   src/master/allocator/sorter/drf/sorter.cpp ed54680cecb637931fc344fbcf8fd3b14cc24295

>   src/master/allocator/sorter/sorter.hpp b3029fcf7342406955760da53f1ae736769f308c 
>   src/tests/hierarchical_allocator_tests.cpp cdf1f15b7802439b28405ca8f6634ce83e886630

>   src/tests/master_allocator_tests.cpp 7b0b786f1c6c53616fd7ae1f7f765752d94a4f83 
>   src/tests/sorter_tests.cpp c93d236b13256f4022a811d019990ef81521aa77 
> 
> 
> Diff: https://reviews.apache.org/r/57254/diff/3/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Neil Conway
> 
>


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