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 70419: Refactor `Sorter::sorted()` to return a stream of clients.
Date Wed, 01 May 2019 17:08:10 GMT

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



Looks good, just hoping we can make the call sites in the allocator a bit more readable


src/master/allocator/mesos/hierarchical.cpp
Line 1826 (original), 1827-1831 (patched)
<https://reviews.apache.org/r/70419/#comment301246>

    Yikes.. this is a lot less readable vs the foreach
    
    Some suggestions:
    
    ```
        Sorter::SortedClients sortedRoles = quotaRoleSorter->sort();
        auto role = sortedRoles.next();
        
        for (; role.isSome(); role = sortedRoles.next()) {
          CHECK(quotaGuarantees.contains(*role));
          ...
    ```
    
    Can't we use role directly with the `*` operator?
    
    Or:
    
    ```
        Sorter::SortedClients sortedRoles = quotaRoleSorter->sort();
        
        while (sortedRoles.hasNext()) {
          const string& role = sortedRoles.next();
          
          CHECK(quotaGuarantees.contains(*role));
          ...
    ```
    
    Ditto for framework sorting



src/master/allocator/sorter/drf/sorter.hpp
Lines 436-438 (patched)
<https://reviews.apache.org/r/70419/#comment301245>

    I think this could be a bit more succinct:
    
    ```
    // TODO(mzhu): Implement incremental sorting by lazily walking the tree.
    ```



src/master/allocator/sorter/sorter.hpp
Lines 157 (patched)
<https://reviews.apache.org/r/70419/#comment301247>

    "complete" doesn't seem to add anything here? Do you mean "forward"?
    
    It seems a lot more straightforward than I had guessed:
    
    https://stackoverflow.com/questions/27604201/implement-lazy-generator-as-forward-iterator-in-c



src/master/allocator/sorter/sorter.hpp
Lines 172 (patched)
<https://reviews.apache.org/r/70419/#comment301248>

    FWICT, SortedClients does not support copy construction or copy assignment since it stores
a unique_ptr, and the code compiles today because all the call sides are doing move construction.
    
    Let's be explicit about what we support and don't support? Something like this?
    
    ```
    virtual SortedClients(SortedClients&&) = default;
    virtual SortedClients& operator=(SortedClients&&) = default;
    
    virtual SortedClients(const SortedClients&) = delete;
    virtual SortedClients& operator=(const SortedClients&) = delete;
    ```
    
    Ditto for the impl?



src/tests/sorter_tests.cpp
Lines 50 (patched)
<https://reviews.apache.org/r/70419/#comment301244>

    Seems like the right thing here is to take an rvalue reference rather than copying?
    
    Doesn't matter much for the test but it also just makes the intent clearer: this will
"consume" the sorted clients by iterating over it, and produce a vector result.
    
    Actually, I'm puzzled at how copying here even works. I suspect what's happening is that
*all* call sites are passing `SortedClients&&` and therefore the move constructor
`SortedClients(SortedClients&&)` is getting invoked and no copying occurs. If a call
site pass a non rvalue, I don't see how it will compile. All the more reason to take an rvalue
here and probably explictly disable copying for now, since that requires us to write the copying
code manually to get it correct.



src/tests/sorter_tests.cpp
Lines 62 (patched)
<https://reviews.apache.org/r/70419/#comment301243>

    add another newline


- Benjamin Mahler


On May 1, 2019, 1:17 a.m., Meng Zhu wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70419/
> -----------------------------------------------------------
> 
> (Updated May 1, 2019, 1:17 a.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Bugs: MESOS-9722
>     https://issues.apache.org/jira/browse/MESOS-9722
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> This patch refactors `Sorter::sorted` to return
> `class SortedClients` instead of a whole vector<string>.
> Callers can then use SortedClients::next() to get the
> next sorted client. This paves the way for sort optimization
> where sorting of the whole clients can be lazily done as callers
> ask for the next client.
> 
> 
> Diffs
> -----
> 
>   src/master/allocator/mesos/hierarchical.cpp 64a076ddd29711437d539a06bb0470755828cc87

>   src/master/allocator/sorter/drf/sorter.hpp 91a9d668b87079158f7072780dc86bb08865166e

>   src/master/allocator/sorter/drf/sorter.cpp 9367469132e426f0b4b66a80ad300c157fba6bf2

>   src/master/allocator/sorter/random/sorter.hpp 55e22d7705f163fe47d5aa47416ee0714c5a87c0

>   src/master/allocator/sorter/random/sorter.cpp 813f5b55d38dd9fa822de53ee944c3f72251a69d

>   src/master/allocator/sorter/sorter.hpp d56a1166a9e82b034564842ac071874ec2885004 
>   src/tests/sorter_tests.cpp 9aee2b41b0d3c978bca6bd2d7ad28e32506a648a 
> 
> 
> Diff: https://reviews.apache.org/r/70419/diff/5/
> 
> 
> Testing
> -------
> 
> make check
> 
> See benchmark result in https://reviews.apache.org/r/70497/
> 
> 
> Thanks,
> 
> Meng Zhu
> 
>


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