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 70498: Simplified Sorter::add(client) implementations.
Date Tue, 23 Apr 2019 02:56:51 GMT

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


Fix it, then Ship it!




Much cleaner, thanks!


src/master/allocator/sorter/drf/sorter.cpp
Line 74 (original), 74 (patched)
<https://reviews.apache.org/r/70498/#comment301050>

    Not sure if this is a behavior change. The current interface does not say anything regarding
adding existing clients.
    
    We should either allow this (maybe a no-op) or make it clear in the interface that clients
should not be added more than once.
    
    Also, since we have the exact same check below (last modified line), this one seems unnecessary.
    
    ditto below in the random sorter



src/master/allocator/sorter/drf/sorter.cpp
Lines 79-82 (patched)
<https://reviews.apache.org/r/70498/#comment301052>

    I was little puzzled here first (e.g. e/f/g should be phase 1(b) + phase 2, while e/f
is phase 1(b) only). 
    
    Maybe better just list the example along with the cases below:
    
      //   Phase 1: Walk down the tree until:
      //     (a) we run out of tokens -> add "." node
      //         e.g. add a
      //     (b) or, we reach a leaf -> transform the leaf into internal + "."
      //         e.g. add e/f, e/f/g, e/f/g/....
      //     (c) or, we're at an internal node but can't find the next child
      //         e.g. add w/x, w/x/y, w/x/y/....
    
    ditto below in the random sorter



src/master/allocator/sorter/drf/sorter.cpp
Lines 89 (patched)
<https://reviews.apache.org/r/70498/#comment301053>

    For the remaining tokens (if any)



src/master/allocator/sorter/drf/sorter.cpp
Lines 94 (patched)
<https://reviews.apache.org/r/70498/#comment301051>

    `tokenIndex` or `depth`?
    
    ditto below in the random sorter


- Meng Zhu


On April 19, 2019, 12:05 p.m., Benjamin Mahler wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70498/
> -----------------------------------------------------------
> 
> (Updated April 19, 2019, 12:05 p.m.)
> 
> 
> Review request for mesos and Meng Zhu.
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> The existing logic is rather hard to understand and there is no
> guiding explanation of the algorithm.
> 
> This re-writes the logic into an easier to understand approach,
> along with a comment at the top that explains the two phase
> algorithm.
> 
> 
> Diffs
> -----
> 
>   src/master/allocator/sorter/drf/sorter.cpp 554ac84ee585d1d07048a58cf7d7d1e6586252ee

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

> 
> 
> Diff: https://reviews.apache.org/r/70498/diff/2/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>


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