> On March 20, 2017, 6:26 p.m., Benjamin Mahler wrote: > > Hm.. seems like there was no test for this "optimization" if the tests > > pass, did you do a scan of the allocator unit tests to be sure we're not > > leaving a stale test lying around? > > Neil Conway wrote: > The change was introduced here, which didn't include any tests: > https://reviews.apache.org/r/41597/ > > Adam B wrote: > A test was added in a follow-up patch: > https://reviews.apache.org/r/41672/diff but that doesn't test the > instant-rebalance behavior specifically. > I'm not sure I understand why you're removing this "optimization". I > believe the original intention was that if an operator updates weights, > he/she would hope to see an immediate impact (since changing `--weights` and > restarting the master implies a rebalance). I suppose a 1s delay is > acceptable, and this does simplify the code. Was there any more specific > motivation here?
Yeah, I was thinking more about this last night and it's quite subtle. In the future, if we add the ability for us to "rebalance" the cluster by rescinding offers to respect changes to the quota or weights, then clearly it seems necessary to induce this immediately when the weights or quotas are changed. The current implementation of quotas and weights however, are such that when quota or weights are modified it has no effect on existing allocations/offers. The only effect is on the distribution of subsequent allocations of available resources. My thinking was therefore that it seemed unfortunate to "flush" the queue of available resources that need allocation because of a weight or quota change, since we mostly care about **re**-balancing and we cannot do that currently. This definitely will not be obvious to people, so we should have comments in the quota and weight setters along the lines of: ``` // Subsequent allocations will reflect the changes to the (weights|quotas). // // If we add the ability for (weight|quota) changes to incur a rebalancing // of the offered resources, then we will need to trigger it immediately // here. For now, since (weight|quota) changes only affect the subsequent // allocations of available resources, we don't interrupt the allocation // loop and simply allow the next run to pick up the changes. ``` - Benjamin ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/57788/#review169477 ----------------------------------------------------------- On March 20, 2017, 6:14 p.m., Neil Conway wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/57788/ > ----------------------------------------------------------- > > (Updated March 20, 2017, 6:14 p.m.) > > > Review request for mesos, Adam B, Benjamin Bannier, Benjamin Mahler, and > Michael Park. > > > Repository: mesos > > > Description > ------- > > When the weight is changed for one or more roles, the allocator will no > longer do a batch allocation. The weight change will be reflected in the > next allocation, which should be sufficient. > > > Diffs > ----- > > src/master/allocator/mesos/hierarchical.cpp > 8d54a8cca1bb478f4437f68c5e14f66a9f9bb9e9 > > > Diff: https://reviews.apache.org/r/57788/diff/1/ > > > Testing > ------- > > `make check` > > > Thanks, > > Neil Conway > >
