mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Neil Conway <neil.con...@gmail.com>
Subject Re: Review Request 57527: Avoided storing weights in the allocator.
Date Mon, 20 Mar 2017 18:15:24 GMT


> On March 18, 2017, 12:09 a.m., Benjamin Mahler wrote:
> > src/master/allocator/mesos/hierarchical.cpp
> > Lines 1329-1355 (original), 1329-1337 (patched)
> > <https://reviews.apache.org/r/57527/diff/3/?file=1666611#file1666611line1329>
> >
> >     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?

Split this into https://reviews.apache.org/r/57788/ and removed the allocation.


> On March 18, 2017, 12:09 a.m., Benjamin Mahler wrote:
> > src/master/allocator/sorter/sorter.hpp
> > Lines 74-78 (patched)
> > <https://reviews.apache.org/r/57527/diff/3/?file=1666614#file1666614line78>
> >
> >     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;
> >     ```

Right -- we don't support it in the sorter because we don't support it in the allocator; we
don't support it in the allocator because we don't allow users to delete/unset weights. I
agree this should be improved (I've griped about it in the past :) -- I opened https://issues.apache.org/jira/browse/MESOS-7267
to track this.

My preference would be to do nothing for now -- if/when we add support for removing weights,
I'd prefer to do it all at once then. Supporting removing weights in some places but not others
seems more confusing to me.


- Neil


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


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