You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Neil Conway <ne...@gmail.com> on 2017/03/20 18:14:50 UTC
Review Request 57788: Changed allocator to skip batch allocate on
weight change.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57788/
-----------------------------------------------------------
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
Re: Review Request 57788: Changed allocator to skip allocation on
weight and quota changes.
Posted by Neil Conway <ne...@gmail.com>.
-----------------------------------------------------------
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
Re: Review Request 57788: Changed allocator to skip batch allocate on
weight change.
Posted by Benjamin Mahler <bm...@apache.org>.
> 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
>
>
Re: Review Request 57788: Changed allocator to skip batch allocate on
weight change.
Posted by Neil Conway <ne...@gmail.com>.
> 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?
The change was introduced here, which didn't include any tests: https://reviews.apache.org/r/41597/
- Neil
-----------------------------------------------------------
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
>
>
Re: Review Request 57788: Changed allocator to skip allocation on
weight and quota changes.
Posted by Alexander Rukletsov <ru...@gmail.com>.
> 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.
Posted by Neil Conway <ne...@gmail.com>.
> 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 batch allocate on
weight change.
Posted by Benjamin Mahler <bm...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57788/#review169477
-----------------------------------------------------------
Ship it!
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?
- Benjamin Mahler
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
>
>