Re: Review Request 57788: Changed allocator to skip allocation on weight and quota changes.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/57788/ --- (Updated April 11, 2017, 9:58 p.m.) Review request for mesos, Adam B, Alexander Rukletsov, Benjamin Bannier, Benjamin Mahler, and Michael Park. Changes --- Rebase? 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 (updated) - src/master/allocator/mesos/hierarchical.cpp 051f749dd5921a322ca930a042c31814616d38f9 src/tests/hierarchical_allocator_tests.cpp 33e7b455f8664858eb4f03727b076a10c80cd6e0 Diff: https://reviews.apache.org/r/57788/diff/3/ Changes: https://reviews.apache.org/r/57788/diff/2-3/ Testing --- `make check` Thanks, Neil Conway
Re: Review Request 57788: Changed allocator to skip allocation on weight and quota changes.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/57788/#review171538 --- Ship it! Ship It! - Adam B On March 22, 2017, 11:36 a.m., Neil Conway wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/57788/ > --- > > (Updated March 22, 2017, 11:36 a.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 > >
Re: Review Request 57788: Changed allocator to skip allocation on weight and quota changes.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/57788/#review170501 --- Ship it! Ship It! - Michael Park On March 22, 2017, 11:36 a.m., Neil Conway wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/57788/ > --- > > (Updated March 22, 2017, 11:36 a.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 > >
Re: Review Request 57788: Changed allocator to skip allocation on weight and quota changes.
> 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 > >
Re: Review Request 57788: Changed allocator to skip allocation on weight and quota changes.
> 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. > ``` Added comments to this effect, and also changed the `setQuota` and `removeQuota` to skip triggering an allocation. - Neil --- 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 > >
Re: Review Request 57788: Changed allocator to skip allocation on weight and quota changes.
--- 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, Benjamin Bannier, Benjamin Mahler, and Michael Park. Changes --- Avoided batch allocation on quota changes, updated comments, tweaked tests. Summary (updated) - Changed allocator to skip allocation on weight and quota changes. Repository: mesos Description (updated) --- 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 (updated) - src/master/allocator/mesos/hierarchical.cpp 8d54a8cca1bb478f4437f68c5e14f66a9f9bb9e9 src/tests/hierarchical_allocator_tests.cpp e343dc37bd7136f0f6dd5dbc22a25cabe715038d Diff: https://reviews.apache.org/r/57788/diff/2/ Changes: https://reviews.apache.org/r/57788/diff/1-2/ Testing --- `make check` Thanks, Neil Conway
