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 70429: Fixed a bug in the random sorter.
Date Thu, 25 Apr 2019 19:09:21 GMT

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


Fix it, then Ship it!





src/master/allocator/sorter/random/sorter.hpp
Lines 135 (patched)
<https://reviews.apache.org/r/70429/#comment301172>

    It's a little odd in this patch to have it as a struct, perhaps just add to the description
(or a TODO here) the intent given the next patch of adding the dirty bit needs to store it
as a member?



src/master/allocator/sorter/random/sorter.cpp
Line 485 (original), 484 (patched)
<https://reviews.apache.org/r/70429/#comment301171>

    I think we may have to std::move here, not sure if the copy will be elided



src/master/allocator/sorter/random/sorter.cpp
Lines 581-582 (patched)
<https://reviews.apache.org/r/70429/#comment301173>

    Just FYI these vectors will always grow in capacity, even without the reserve calls here,
seems ok for now but maybe we should write a note about it or something



src/master/allocator/sorter/random/sorter.cpp
Lines 584-590 (patched)
<https://reviews.apache.org/r/70429/#comment301174>

    I think we could boil this down to just the formula?
    
    ```
                                                      weight(node)
    rWeight(node) = rWeight(parent) *  ------------------------------------------
                                       weight(node) + SUM(weight(active siblings))
    ```



src/master/allocator/sorter/random/sorter.cpp
Lines 596 (patched)
<https://reviews.apache.org/r/70429/#comment301177>

    How about s/totalWeights/siblingWeights/?
    
    We could either note that the siblingWeights includes the node itself, or do these adjustments:
    
    ```
          double relativeWeight = parentRelativeWeight *
            (sorter->getWeight(node) / (sorter->getWeight(node) + siblingWeights));
    ```
    
    ```
              calculateRelativeWeights(child, totalWeights_ - sorter->getWeight(child),
relativeWeight);
    ```
    
    ```
    calculateRelativeWeights(sorter->root, 0.0, 1.0);
    ```
    
    The version with adjustments seems to be easier to understand based on the formula above
without much additional complexity?



src/master/allocator/sorter/random/sorter.cpp
Lines 597 (patched)
<https://reviews.apache.org/r/70429/#comment301176>

    !activeInternals.contains(node)
    
    (will work once we use hashset)
    
    Also, all these checks make me think we should have a lambda?
    
    ```
    auto isActive = [&](Node* node) {
      return node->kind == Node::ACTIVE_LEAF || activeInternals.contains(child);
    };
    ```
    
    Then just call it below?



src/master/allocator/sorter/random/sorter.cpp
Lines 615-616 (patched)
<https://reviews.apache.org/r/70429/#comment301180>

    This would read a bit easier as isActive(child), see the suggestion above



src/master/allocator/sorter/random/sorter.cpp
Lines 616 (patched)
<https://reviews.apache.org/r/70429/#comment301178>

    activeInternals.contains(child)



src/master/allocator/sorter/random/sorter.cpp
Lines 623 (patched)
<https://reviews.apache.org/r/70429/#comment301179>

    activeInternals.contains(child)


- Benjamin Mahler


On April 24, 2019, 10:34 p.m., Meng Zhu wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70429/
> -----------------------------------------------------------
> 
> (Updated April 24, 2019, 10:34 p.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Bugs: MESOS-9733
>     https://issues.apache.org/jira/browse/MESOS-9733
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Currently, in the presence of hierarchical roles, the
> random sorter shuffles roles level by level and then pick
> the active leave nodes using DFS. This could generate
> non-uniform random result since active leaves in a subtree
> are always picked together.
> 
> This patch fixes the issue by first calculating the relative
> weights of each active leaf node and shuffle all of them
> only once.
> 
> 
> Diffs
> -----
> 
>   src/master/allocator/sorter/random/sorter.hpp 125ce84761e4c930370912151700ddda35d7b6c1

>   src/master/allocator/sorter/random/sorter.cpp bbe130dbf3b158ea14f9572bc5d14200fcd85127

>   src/tests/sorter_tests.cpp 91bfde01b579a9892215593ab8d8b2d096788278 
> 
> 
> Diff: https://reviews.apache.org/r/70429/diff/4/
> 
> 
> Testing
> -------
> 
> make check
> Tests for hierarchical roles in subsequent patches
> 
> Benchmarking:
> Optimized build with QuotaParam/BENCHMARK_HierarchicalAllocator_WithQuotaParam.LargeAndSmallQuota/2
> 
> ## Before:
> Added 3000 agents in 85.844373ms
> Added 3000 frameworks in 19.713969615secs
> Benchmark setup: 3000 agents, 3000 roles, 3000 frameworks
> Made 3500 allocations in 13.690538305secs
> Made 0 allocation in 9.76855825secs
> 
> ## After:
> Added 3000 agents in 93.482508ms
> Added 3000 frameworks in 19.193235872secs
> Benchmark setup: 3000 agents, 3000 roles, 3000 frameworks
> Made 3500 allocations in 13.530734654secs
> Made 0 allocation in 10.338803223secs
> 
> 
> Thanks,
> 
> Meng Zhu
> 
>


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