> 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 > >
