> 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? > > Benjamin Mahler wrote: > 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. > ``` > > Neil Conway wrote: > Added comments to this effect, and also changed the `setQuota` and > `removeQuota` to skip triggering an allocation.
I think it's fine not to trigger event-based allocations for quota. - Alexander ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/57788/#review169477 ----------------------------------------------------------- On March 22, 2017, 6:36 p.m., Neil Conway wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/57788/ > ----------------------------------------------------------- > > (Updated March 22, 2017, 6:36 p.m.) > > > Review request for mesos, Adam B, Alexander Rukletsov, Benjamin Bannier, > Benjamin Mahler, and Michael Park. > > > Repository: mesos > > > Description > ------- > > Changing weight or quota will no longer trigger a batch allocation. > Since the allocator does not rebalance currently offered resources to > reflect changes to weight or quota, doing a batch allocation is not > useful; instead, the updated quota/weight values will be reflected on > the next allocation. > > > Diffs > ----- > > src/master/allocator/mesos/hierarchical.cpp > 8d54a8cca1bb478f4437f68c5e14f66a9f9bb9e9 > src/tests/hierarchical_allocator_tests.cpp > e343dc37bd7136f0f6dd5dbc22a25cabe715038d > > > Diff: https://reviews.apache.org/r/57788/diff/2/ > > > Testing > ------- > > `make check` > > > Thanks, > > Neil Conway > >
