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