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 70498: Simplified Sorter::add(client) implementations.
Date Wed, 24 Apr 2019 21:59:44 GMT


> On April 23, 2019, 2:56 a.m., Meng Zhu wrote:
> > src/master/allocator/sorter/drf/sorter.cpp
> > Line 74 (original), 74 (patched)
> > <https://reviews.apache.org/r/70498/diff/2/?file=2140338#file2140338line74>
> >
> >     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

It's not a behavior change AFAICT, you'll notice that the current code performs this check
*at the end* and does not touch the clients map until that point, whereas it seems more clear
to perform it at the start to enforce this. I also perform the check at the end now, since
there is some modification of the clients map in phase 1, so want to sanity check it after
the algorithm as well.

Agreed the interface should document that you can't add an already existing client or remove
a missing client.


> On April 23, 2019, 2:56 a.m., Meng Zhu wrote:
> > src/master/allocator/sorter/drf/sorter.cpp
> > Lines 79-82 (patched)
> > <https://reviews.apache.org/r/70498/diff/2/?file=2140338#file2140338line80>
> >
> >     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

All of them go through phase 2 (note that the code does not have any conditional around whether
phase 2 runs), it's just that the number of remaining tokens might be 0.


> On April 23, 2019, 2:56 a.m., Meng Zhu wrote:
> > src/master/allocator/sorter/drf/sorter.cpp
> > Lines 89 (patched)
> > <https://reviews.apache.org/r/70498/diff/2/?file=2140338#file2140338line90>
> >
> >     For the remaining tokens (if any)

Ok!


> On April 23, 2019, 2:56 a.m., Meng Zhu wrote:
> > src/master/allocator/sorter/drf/sorter.cpp
> > Lines 94 (patched)
> > <https://reviews.apache.org/r/70498/diff/2/?file=2140338#file2140338line95>
> >
> >     `tokenIndex` or `depth`?
> >     
> >     ditto below in the random sorter

Hm.. on second though, I'll use an iterator, that way we can keep calling it `token`!


- Benjamin


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


On April 19, 2019, 7: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, 7: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