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 57527: Avoided storing weights in the allocator.
Date Sat, 18 Mar 2017 00:09:49 GMT

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



For posterity, Neil and I discussed at length offline whether we want to de-couple weights
from clients. It seems that in the hierarchical model, we have to store the weights beyond
the existing lifecycle of a client, since the (grand-)(grand-)(...)parent weights are needed
when a client arrives.

One way to have this fit cleanly is to represent the "paths with weights" as inactive clients
(rather than storing weights separately) and have the sorter clean clients up once no more
information is needed (i.e. the client has non default values: active, has resources allocated,
has children, has weight, etc.). I didn't manage to convince Neil that this was indeed easier
for people to understand :), but since this requires some refactoring to accomplish anyway
we'll stick with the current approach and we can explore the improvements later.

Neil: This looks good, modulo the two issues below.


src/master/allocator/mesos/hierarchical.cpp
Lines 1329-1355 (original), 1329-1337 (patched)
<https://reviews.apache.org/r/57527/#comment241705>

    Hm.. this seems somewhat snuck in to this patch. It's not clear if we should even be triggering
an allocation here, my inclination is no, since the weight update will be reflected in the
next allocation and there's nothing here that warrants an immediate allocation.
    
    In any case, can you pull it out in front of this review?



src/master/allocator/sorter/sorter.hpp
Lines 74-78 (patched)
<https://reviews.apache.org/r/57527/#comment241706>

    I guess I was more curious as to why we don't support it, is it because we don't support
removing unsetting weights from the mesos api? If so, wow :) and also, shouldn't we just support
it here in anticipation? Seems pretty straightforward to me to just avoid this confusion:
    
    ```
      virtual void setWeight(const std::string& client, double weight) = 0;
      virtual void unsetWeight(const std::string& client) = 0;
    ```



src/master/allocator/sorter/sorter.hpp
Lines 74-75 (patched)
<https://reviews.apache.org/r/57527/#comment241709>

    Per our offline conversation and the thread earlier in this review, the fact that we update
weights for clients that are not in the sorter seems odd, but I understand it's helpful for
getting the hierarchical implementation to fit in the existing sorter API. We can explore
simplifying it per my suggestion above later.


- Benjamin Mahler


On March 17, 2017, 1:12 a.m., Neil Conway wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57527/
> -----------------------------------------------------------
> 
> (Updated March 17, 2017, 1:12 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Benjamin Mahler, and Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Previously, DRFSorter only kept track of weights for clients currently
> in the sorter; the allocator supplied the current weight when adding a
> client to the sorter.
> 
> This commit changes the sorter to keep track of all weights, not just
> those for the active clients in the sorter. The allocator can now just
> pass along role weights to the role sorters, rather than needing to
> track them itself.
> 
> This commit changes the sorter API.
> 
> 
> Diffs
> -----
> 
>   src/master/allocator/mesos/hierarchical.hpp f84b0574ce9a392c9528c87b04b01dbb2053cff7

>   src/master/allocator/mesos/hierarchical.cpp 8d54a8cca1bb478f4437f68c5e14f66a9f9bb9e9

>   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/sorter_tests.cpp ec0636beb936d46a253d19322f2157abe95156b6 
> 
> 
> Diff: https://reviews.apache.org/r/57527/diff/3/
> 
> 
> Testing
> -------
> 
> `make check`
> 
> 
> Thanks,
> 
> Neil Conway
> 
>


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