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/13 18:04:02 UTC
Review Request 57564: Changed DRFSorter's representation of inactive
clients.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57564/
-----------------------------------------------------------
Review request for mesos, Benjamin Bannier, Benjamin Mahler, and Michael Park.
Repository: mesos
Description
-------
DRFSorter previously removed inactive clients from the `clients`
collection, and then re-added clients when they were reactivated. This
resulted in resetting the allocation count for the client, which is
unfortunate. This scheme would also be more difficult to adapt to
hierarchical sorting.
This commit changes DRFSorter to continue to store inactive clients in
the `clients`; inactive clients are indicated by a new field in the
`Client` struct, and are omitted from the return value of
`DRFSorter::sort`.
Diffs
-----
src/master/allocator/sorter/drf/sorter.hpp 76329220e1115c1de7810fb69b943c78c078be59
src/master/allocator/sorter/drf/sorter.cpp ed54680cecb637931fc344fbcf8fd3b14cc24295
src/tests/sorter_tests.cpp ec0636beb936d46a253d19322f2157abe95156b6
Diff: https://reviews.apache.org/r/57564/diff/1/
Testing
-------
`make check`
Thanks,
Neil Conway
Re: Review Request 57564: Changed DRFSorter's representation of
inactive clients.
Posted by Neil Conway <ne...@gmail.com>.
> On March 29, 2017, 10:42 p.m., Benjamin Mahler wrote:
> > src/master/allocator/sorter/drf/sorter.hpp
> > Lines 214-216 (original), 215-217 (patched)
> > <https://reviews.apache.org/r/57564/diff/1/?file=1662777#file1662777line215>
> >
> > This comment is no longer accurate, right? We used to store a distinct structure purely because of the deactivated clients being absent from the `clients` map. Now that this is no longer the case, it seems like this patch should be removing this additional data structure (which needs to be kept in sync) and just storing `Allocation` within Client.
> >
> > Won't we need to do this anyway for the hierarchy change? Seems clearer to me to make that change in this patch.
>
> Neil Conway wrote:
> Right, the comment was stale (although it was removed in the next review in the chain anyway); I updated it.
>
> I'll look into splitting the `Allocation` change out into a separate review.
I removed the `allocations` map in a separate review: https://reviews.apache.org/r/58110/
- Neil
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57564/#review170491
-----------------------------------------------------------
On March 30, 2017, 5:58 p.m., Neil Conway wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57564/
> -----------------------------------------------------------
>
> (Updated March 30, 2017, 5:58 p.m.)
>
>
> Review request for mesos, Benjamin Bannier, Benjamin Mahler, and Michael Park.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> DRFSorter previously removed inactive clients from the `clients`
> collection, and then re-added clients when they were reactivated. This
> resulted in resetting the allocation count for the client, which is
> unfortunate. This scheme would also be more difficult to adapt to
> hierarchical sorting.
>
> This commit changes DRFSorter to continue to store inactive clients in
> the `clients`; inactive clients are indicated by a new field in the
> `Client` struct, and are omitted from the return value of
> `DRFSorter::sort`.
>
>
> Diffs
> -----
>
> src/master/allocator/sorter/drf/sorter.hpp 76329220e1115c1de7810fb69b943c78c078be59
> src/master/allocator/sorter/drf/sorter.cpp ed54680cecb637931fc344fbcf8fd3b14cc24295
> src/tests/sorter_tests.cpp ec0636beb936d46a253d19322f2157abe95156b6
>
>
> Diff: https://reviews.apache.org/r/57564/diff/2/
>
>
> Testing
> -------
>
> `make check`
>
>
> Thanks,
>
> Neil Conway
>
>
Re: Review Request 57564: Changed DRFSorter's representation of
inactive clients.
Posted by Benjamin Mahler <bm...@apache.org>.
> On March 29, 2017, 10:42 p.m., Benjamin Mahler wrote:
> > src/master/allocator/sorter/drf/sorter.hpp
> > Lines 214-216 (original), 215-217 (patched)
> > <https://reviews.apache.org/r/57564/diff/1/?file=1662777#file1662777line215>
> >
> > This comment is no longer accurate, right? We used to store a distinct structure purely because of the deactivated clients being absent from the `clients` map. Now that this is no longer the case, it seems like this patch should be removing this additional data structure (which needs to be kept in sync) and just storing `Allocation` within Client.
> >
> > Won't we need to do this anyway for the hierarchy change? Seems clearer to me to make that change in this patch.
>
> Neil Conway wrote:
> Right, the comment was stale (although it was removed in the next review in the chain anyway); I updated it.
>
> I'll look into splitting the `Allocation` change out into a separate review.
>
> Neil Conway wrote:
> I removed the `allocations` map in a separate review: https://reviews.apache.org/r/58110/
Ok, sounds good, do you want to leave a TODO in this patch?
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57564/#review170491
-----------------------------------------------------------
On March 31, 2017, 8:15 p.m., Neil Conway wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57564/
> -----------------------------------------------------------
>
> (Updated March 31, 2017, 8:15 p.m.)
>
>
> Review request for mesos, Benjamin Bannier, Benjamin Mahler, and Michael Park.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> DRFSorter previously removed inactive clients from the `clients`
> collection, and then re-added clients when they were reactivated. This
> resulted in resetting the allocation count for the client, which is
> unfortunate. This scheme would also be more difficult to adapt to
> hierarchical sorting.
>
> This commit changes DRFSorter to continue to store inactive clients in
> the `clients`; inactive clients are indicated by a new field in the
> `Client` struct, and are omitted from the return value of
> `DRFSorter::sort`.
>
>
> Diffs
> -----
>
> src/master/allocator/sorter/drf/sorter.hpp 76329220e1115c1de7810fb69b943c78c078be59
> src/master/allocator/sorter/drf/sorter.cpp ed54680cecb637931fc344fbcf8fd3b14cc24295
> src/tests/sorter_tests.cpp ec0636beb936d46a253d19322f2157abe95156b6
>
>
> Diff: https://reviews.apache.org/r/57564/diff/3/
>
>
> Testing
> -------
>
> `make check`
>
>
> Thanks,
>
> Neil Conway
>
>
Re: Review Request 57564: Changed DRFSorter's representation of
inactive clients.
Posted by Neil Conway <ne...@gmail.com>.
> On March 29, 2017, 10:42 p.m., Benjamin Mahler wrote:
> > src/master/allocator/sorter/drf/sorter.hpp
> > Lines 214-216 (original), 215-217 (patched)
> > <https://reviews.apache.org/r/57564/diff/1/?file=1662777#file1662777line215>
> >
> > This comment is no longer accurate, right? We used to store a distinct structure purely because of the deactivated clients being absent from the `clients` map. Now that this is no longer the case, it seems like this patch should be removing this additional data structure (which needs to be kept in sync) and just storing `Allocation` within Client.
> >
> > Won't we need to do this anyway for the hierarchy change? Seems clearer to me to make that change in this patch.
Right, the comment was stale (although it was removed in the next review in the chain anyway); I updated it.
I'll look into splitting the `Allocation` change out into a separate review.
- Neil
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57564/#review170491
-----------------------------------------------------------
On March 13, 2017, 6:04 p.m., Neil Conway wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57564/
> -----------------------------------------------------------
>
> (Updated March 13, 2017, 6:04 p.m.)
>
>
> Review request for mesos, Benjamin Bannier, Benjamin Mahler, and Michael Park.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> DRFSorter previously removed inactive clients from the `clients`
> collection, and then re-added clients when they were reactivated. This
> resulted in resetting the allocation count for the client, which is
> unfortunate. This scheme would also be more difficult to adapt to
> hierarchical sorting.
>
> This commit changes DRFSorter to continue to store inactive clients in
> the `clients`; inactive clients are indicated by a new field in the
> `Client` struct, and are omitted from the return value of
> `DRFSorter::sort`.
>
>
> Diffs
> -----
>
> src/master/allocator/sorter/drf/sorter.hpp 76329220e1115c1de7810fb69b943c78c078be59
> src/master/allocator/sorter/drf/sorter.cpp ed54680cecb637931fc344fbcf8fd3b14cc24295
> src/tests/sorter_tests.cpp ec0636beb936d46a253d19322f2157abe95156b6
>
>
> Diff: https://reviews.apache.org/r/57564/diff/1/
>
>
> Testing
> -------
>
> `make check`
>
>
> Thanks,
>
> Neil Conway
>
>
Re: Review Request 57564: Changed DRFSorter's representation of
inactive clients.
Posted by Benjamin Mahler <bm...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57564/#review170491
-----------------------------------------------------------
src/master/allocator/sorter/drf/sorter.hpp
Lines 214-216 (original), 215-217 (patched)
<https://reviews.apache.org/r/57564/#comment243325>
This comment is no longer accurate, right? We used to store a distinct structure purely because of the deactivated clients being absent from the `clients` map. Now that this is no longer the case, it seems like this patch should be removing this additional data structure (which needs to be kept in sync) and just storing `Allocation` within Client.
Won't we need to do this anyway for the hierarchy change? Seems clearer to me to make that change in this patch.
src/tests/sorter_tests.cpp
Lines 243 (patched)
<https://reviews.apache.org/r/57564/#comment243324>
Thanks for adding these comments!
src/tests/sorter_tests.cpp
Line 267 (original), 268 (patched)
<https://reviews.apache.org/r/57564/#comment243322>
Looks like c is supposed to have a count of 5 here?
src/tests/sorter_tests.cpp
Lines 282 (patched)
<https://reviews.apache.org/r/57564/#comment243323>
Ditto here?
- Benjamin Mahler
On March 13, 2017, 6:04 p.m., Neil Conway wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57564/
> -----------------------------------------------------------
>
> (Updated March 13, 2017, 6:04 p.m.)
>
>
> Review request for mesos, Benjamin Bannier, Benjamin Mahler, and Michael Park.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> DRFSorter previously removed inactive clients from the `clients`
> collection, and then re-added clients when they were reactivated. This
> resulted in resetting the allocation count for the client, which is
> unfortunate. This scheme would also be more difficult to adapt to
> hierarchical sorting.
>
> This commit changes DRFSorter to continue to store inactive clients in
> the `clients`; inactive clients are indicated by a new field in the
> `Client` struct, and are omitted from the return value of
> `DRFSorter::sort`.
>
>
> Diffs
> -----
>
> src/master/allocator/sorter/drf/sorter.hpp 76329220e1115c1de7810fb69b943c78c078be59
> src/master/allocator/sorter/drf/sorter.cpp ed54680cecb637931fc344fbcf8fd3b14cc24295
> src/tests/sorter_tests.cpp ec0636beb936d46a253d19322f2157abe95156b6
>
>
> Diff: https://reviews.apache.org/r/57564/diff/1/
>
>
> Testing
> -------
>
> `make check`
>
>
> Thanks,
>
> Neil Conway
>
>
Re: Review Request 57564: Changed DRFSorter's representation of
inactive clients.
Posted by Neil Conway <ne...@gmail.com>.
> On March 17, 2017, 1:33 a.m., Benjamin Mahler wrote:
> > Hm.. this seems to introduce a performance regression in the case where there are many inactive roles in the system? I assume it's difficult to avoid in the hierarchical case? Otherwise, this looks good.
True -- if there are many inactive clients, we'll now calculate their share and include them in the `std::set`, whereas we wouldn't do so before (conversely, activating and deactivating clients is a lot faster than it used to be). My guess is that the bottlenecks in sorter performance likely lie elsewhere (e.g., updating resources, generating the entire `vector` in `sort()` when the allocator might only need the first few entries).
We could avoid this overhead by skipping `calculateShare` for inactive clients, and then dirtying the whole sorter when a client is activated. My guess is that this isn't a net win (if you have a lot of inactive clients, it seems unfortunate to dirty the entire sorter whenever a client becomes active), but I can do some benchmarks if you think this case is important.
> On March 17, 2017, 1:33 a.m., Benjamin Mahler wrote:
> > src/master/allocator/sorter/drf/sorter.cpp
> > Line 110 (original), 112-113 (patched)
> > <https://reviews.apache.org/r/57564/diff/1/?file=1662778#file1662778line114>
> >
> > Why does it need to be re-inserted? Is this just to be prudent in case 'active' has an effect on the sorting? Or is it needed today? A comment would be great.
The `std::set` interface doesn't let you modify elements directly. We could declare `active` to be `mutable`, which would avoid the need for the re-insertion. (This code is changed in the next review anyway.)
> On March 17, 2017, 1:33 a.m., Benjamin Mahler wrote:
> > src/master/allocator/sorter/drf/sorter.cpp
> > Lines 156 (patched)
> > <https://reviews.apache.org/r/57564/diff/1/?file=1662778#file1662778line163>
> >
> > Why the additional scope here? Seems a bit inconsistent
Introducing a nested scope for distinct "steps" in a function can be helpful, IMO. In this case, we're introducing variables with names like `it` and `client`; it seems nice to avoid having those variables be in-scope for the entire function.
- Neil
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57564/#review169245
-----------------------------------------------------------
On March 13, 2017, 6:04 p.m., Neil Conway wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57564/
> -----------------------------------------------------------
>
> (Updated March 13, 2017, 6:04 p.m.)
>
>
> Review request for mesos, Benjamin Bannier, Benjamin Mahler, and Michael Park.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> DRFSorter previously removed inactive clients from the `clients`
> collection, and then re-added clients when they were reactivated. This
> resulted in resetting the allocation count for the client, which is
> unfortunate. This scheme would also be more difficult to adapt to
> hierarchical sorting.
>
> This commit changes DRFSorter to continue to store inactive clients in
> the `clients`; inactive clients are indicated by a new field in the
> `Client` struct, and are omitted from the return value of
> `DRFSorter::sort`.
>
>
> Diffs
> -----
>
> src/master/allocator/sorter/drf/sorter.hpp 76329220e1115c1de7810fb69b943c78c078be59
> src/master/allocator/sorter/drf/sorter.cpp ed54680cecb637931fc344fbcf8fd3b14cc24295
> src/tests/sorter_tests.cpp ec0636beb936d46a253d19322f2157abe95156b6
>
>
> Diff: https://reviews.apache.org/r/57564/diff/1/
>
>
> Testing
> -------
>
> `make check`
>
>
> Thanks,
>
> Neil Conway
>
>
Re: Review Request 57564: Changed DRFSorter's representation of
inactive clients.
Posted by Benjamin Mahler <bm...@apache.org>.
> On March 17, 2017, 1:33 a.m., Benjamin Mahler wrote:
> > Hm.. this seems to introduce a performance regression in the case where there are many inactive roles in the system? I assume it's difficult to avoid in the hierarchical case? Otherwise, this looks good.
>
> Neil Conway wrote:
> True -- if there are many inactive clients, we'll now calculate their share and include them in the `std::set`, whereas we wouldn't do so before (conversely, activating and deactivating clients is a lot faster than it used to be). My guess is that the bottlenecks in sorter performance likely lie elsewhere (e.g., updating resources, generating the entire `vector` in `sort()` when the allocator might only need the first few entries).
>
> We could avoid this overhead by skipping `calculateShare` for inactive clients, and then dirtying the whole sorter when a client is activated. My guess is that this isn't a net win (if you have a lot of inactive clients, it seems unfortunate to dirty the entire sorter whenever a client becomes active), but I can do some benchmarks if you think this case is important.
Another alternative to avoid dirtying the whole sorter when a client is activated is to a per-client dirty bit, or `Option<double> share` where if set, it is accurate (and we can unset to induce the lazy calculation). `dirty = true` would be equivalent to clearing all of the clients' shares. Thinking about it, the `Option<double>` seems simpler to understand as well (no way for a stale value to be there).
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57564/#review169245
-----------------------------------------------------------
On March 13, 2017, 6:04 p.m., Neil Conway wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57564/
> -----------------------------------------------------------
>
> (Updated March 13, 2017, 6:04 p.m.)
>
>
> Review request for mesos, Benjamin Bannier, Benjamin Mahler, and Michael Park.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> DRFSorter previously removed inactive clients from the `clients`
> collection, and then re-added clients when they were reactivated. This
> resulted in resetting the allocation count for the client, which is
> unfortunate. This scheme would also be more difficult to adapt to
> hierarchical sorting.
>
> This commit changes DRFSorter to continue to store inactive clients in
> the `clients`; inactive clients are indicated by a new field in the
> `Client` struct, and are omitted from the return value of
> `DRFSorter::sort`.
>
>
> Diffs
> -----
>
> src/master/allocator/sorter/drf/sorter.hpp 76329220e1115c1de7810fb69b943c78c078be59
> src/master/allocator/sorter/drf/sorter.cpp ed54680cecb637931fc344fbcf8fd3b14cc24295
> src/tests/sorter_tests.cpp ec0636beb936d46a253d19322f2157abe95156b6
>
>
> Diff: https://reviews.apache.org/r/57564/diff/1/
>
>
> Testing
> -------
>
> `make check`
>
>
> Thanks,
>
> Neil Conway
>
>
Re: Review Request 57564: Changed DRFSorter's representation of
inactive clients.
Posted by Benjamin Mahler <bm...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57564/#review169245
-----------------------------------------------------------
Hm.. this seems to introduce a performance regression in the case where there are many inactive roles in the system? I assume it's difficult to avoid in the hierarchical case? Otherwise, this looks good.
src/master/allocator/sorter/drf/sorter.cpp
Line 110 (original), 112-113 (patched)
<https://reviews.apache.org/r/57564/#comment241592>
Why does it need to be re-inserted? Is this just to be prudent in case 'active' has an effect on the sorting? Or is it needed today? A comment would be great.
src/master/allocator/sorter/drf/sorter.cpp
Line 126 (original), 129-130 (patched)
<https://reviews.apache.org/r/57564/#comment241593>
Ditto here.
src/master/allocator/sorter/drf/sorter.cpp
Lines 156 (patched)
<https://reviews.apache.org/r/57564/#comment241594>
Why the additional scope here? Seems a bit inconsistent
- Benjamin Mahler
On March 13, 2017, 6:04 p.m., Neil Conway wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57564/
> -----------------------------------------------------------
>
> (Updated March 13, 2017, 6:04 p.m.)
>
>
> Review request for mesos, Benjamin Bannier, Benjamin Mahler, and Michael Park.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> DRFSorter previously removed inactive clients from the `clients`
> collection, and then re-added clients when they were reactivated. This
> resulted in resetting the allocation count for the client, which is
> unfortunate. This scheme would also be more difficult to adapt to
> hierarchical sorting.
>
> This commit changes DRFSorter to continue to store inactive clients in
> the `clients`; inactive clients are indicated by a new field in the
> `Client` struct, and are omitted from the return value of
> `DRFSorter::sort`.
>
>
> Diffs
> -----
>
> src/master/allocator/sorter/drf/sorter.hpp 76329220e1115c1de7810fb69b943c78c078be59
> src/master/allocator/sorter/drf/sorter.cpp ed54680cecb637931fc344fbcf8fd3b14cc24295
> src/tests/sorter_tests.cpp ec0636beb936d46a253d19322f2157abe95156b6
>
>
> Diff: https://reviews.apache.org/r/57564/diff/1/
>
>
> Testing
> -------
>
> `make check`
>
>
> Thanks,
>
> Neil Conway
>
>
Re: Review Request 57564: Changed DRFSorter's representation of
inactive clients.
Posted by Neil Conway <ne...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57564/
-----------------------------------------------------------
(Updated March 31, 2017, 9:33 p.m.)
Review request for mesos, Benjamin Bannier, Benjamin Mahler, and Michael Park.
Changes
-------
Add TODO, per review.
Repository: mesos
Description
-------
DRFSorter previously removed inactive clients from the `clients`
collection, and then re-added clients when they were reactivated. This
resulted in resetting the allocation count for the client, which is
unfortunate. This scheme would also be more difficult to adapt to
hierarchical sorting.
This commit changes DRFSorter to continue to store inactive clients in
the `clients`; inactive clients are indicated by a new field in the
`Client` struct, and are omitted from the return value of
`DRFSorter::sort`.
Diffs (updated)
-----
src/master/allocator/sorter/drf/sorter.hpp 76329220e1115c1de7810fb69b943c78c078be59
src/master/allocator/sorter/drf/sorter.cpp ed54680cecb637931fc344fbcf8fd3b14cc24295
src/tests/sorter_tests.cpp ec0636beb936d46a253d19322f2157abe95156b6
Diff: https://reviews.apache.org/r/57564/diff/4/
Changes: https://reviews.apache.org/r/57564/diff/3-4/
Testing
-------
`make check`
Thanks,
Neil Conway
Re: Review Request 57564: Changed DRFSorter's representation of
inactive clients.
Posted by Neil Conway <ne...@gmail.com>.
> On March 31, 2017, 9:24 p.m., Benjamin Mahler wrote:
> > src/master/allocator/sorter/drf/sorter.hpp
> > Lines 214-217 (original), 215-216 (patched)
> > <https://reviews.apache.org/r/57564/diff/3/?file=1682653#file1682653line215>
> >
> > A TODO to consolidate this into the client struct would be helpful for posterity
Done.
- Neil
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57564/#review170780
-----------------------------------------------------------
On March 31, 2017, 9:33 p.m., Neil Conway wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57564/
> -----------------------------------------------------------
>
> (Updated March 31, 2017, 9:33 p.m.)
>
>
> Review request for mesos, Benjamin Bannier, Benjamin Mahler, and Michael Park.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> DRFSorter previously removed inactive clients from the `clients`
> collection, and then re-added clients when they were reactivated. This
> resulted in resetting the allocation count for the client, which is
> unfortunate. This scheme would also be more difficult to adapt to
> hierarchical sorting.
>
> This commit changes DRFSorter to continue to store inactive clients in
> the `clients`; inactive clients are indicated by a new field in the
> `Client` struct, and are omitted from the return value of
> `DRFSorter::sort`.
>
>
> Diffs
> -----
>
> src/master/allocator/sorter/drf/sorter.hpp 76329220e1115c1de7810fb69b943c78c078be59
> src/master/allocator/sorter/drf/sorter.cpp ed54680cecb637931fc344fbcf8fd3b14cc24295
> src/tests/sorter_tests.cpp ec0636beb936d46a253d19322f2157abe95156b6
>
>
> Diff: https://reviews.apache.org/r/57564/diff/4/
>
>
> Testing
> -------
>
> `make check`
>
>
> Thanks,
>
> Neil Conway
>
>
Re: Review Request 57564: Changed DRFSorter's representation of
inactive clients.
Posted by Benjamin Mahler <bm...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57564/#review170780
-----------------------------------------------------------
Ship it!
src/master/allocator/sorter/drf/sorter.hpp
Lines 214-217 (original), 215-216 (patched)
<https://reviews.apache.org/r/57564/#comment243659>
A TODO to consolidate this into the client struct would be helpful for posterity
- Benjamin Mahler
On March 31, 2017, 8:15 p.m., Neil Conway wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57564/
> -----------------------------------------------------------
>
> (Updated March 31, 2017, 8:15 p.m.)
>
>
> Review request for mesos, Benjamin Bannier, Benjamin Mahler, and Michael Park.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> DRFSorter previously removed inactive clients from the `clients`
> collection, and then re-added clients when they were reactivated. This
> resulted in resetting the allocation count for the client, which is
> unfortunate. This scheme would also be more difficult to adapt to
> hierarchical sorting.
>
> This commit changes DRFSorter to continue to store inactive clients in
> the `clients`; inactive clients are indicated by a new field in the
> `Client` struct, and are omitted from the return value of
> `DRFSorter::sort`.
>
>
> Diffs
> -----
>
> src/master/allocator/sorter/drf/sorter.hpp 76329220e1115c1de7810fb69b943c78c078be59
> src/master/allocator/sorter/drf/sorter.cpp ed54680cecb637931fc344fbcf8fd3b14cc24295
> src/tests/sorter_tests.cpp ec0636beb936d46a253d19322f2157abe95156b6
>
>
> Diff: https://reviews.apache.org/r/57564/diff/3/
>
>
> Testing
> -------
>
> `make check`
>
>
> Thanks,
>
> Neil Conway
>
>
Re: Review Request 57564: Changed DRFSorter's representation of
inactive clients.
Posted by Neil Conway <ne...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57564/
-----------------------------------------------------------
(Updated March 31, 2017, 8:15 p.m.)
Review request for mesos, Benjamin Bannier, Benjamin Mahler, and Michael Park.
Changes
-------
Remove `std::vector::reserve()` per review.
Repository: mesos
Description
-------
DRFSorter previously removed inactive clients from the `clients`
collection, and then re-added clients when they were reactivated. This
resulted in resetting the allocation count for the client, which is
unfortunate. This scheme would also be more difficult to adapt to
hierarchical sorting.
This commit changes DRFSorter to continue to store inactive clients in
the `clients`; inactive clients are indicated by a new field in the
`Client` struct, and are omitted from the return value of
`DRFSorter::sort`.
Diffs (updated)
-----
src/master/allocator/sorter/drf/sorter.hpp 76329220e1115c1de7810fb69b943c78c078be59
src/master/allocator/sorter/drf/sorter.cpp ed54680cecb637931fc344fbcf8fd3b14cc24295
src/tests/sorter_tests.cpp ec0636beb936d46a253d19322f2157abe95156b6
Diff: https://reviews.apache.org/r/57564/diff/3/
Changes: https://reviews.apache.org/r/57564/diff/2-3/
Testing
-------
`make check`
Thanks,
Neil Conway
Re: Review Request 57564: Changed DRFSorter's representation of
inactive clients.
Posted by Michael Park <mp...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57564/#review170754
-----------------------------------------------------------
Fix it, then Ship it!
src/master/allocator/sorter/drf/sorter.cpp
Line 412 (original), 416 (patched)
<https://reviews.apache.org/r/57564/#comment243637>
Let's remove this now that we're not necessarily pushing `client.size()` elements here.
- Michael Park
On March 30, 2017, 10:58 a.m., Neil Conway wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57564/
> -----------------------------------------------------------
>
> (Updated March 30, 2017, 10:58 a.m.)
>
>
> Review request for mesos, Benjamin Bannier, Benjamin Mahler, and Michael Park.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> DRFSorter previously removed inactive clients from the `clients`
> collection, and then re-added clients when they were reactivated. This
> resulted in resetting the allocation count for the client, which is
> unfortunate. This scheme would also be more difficult to adapt to
> hierarchical sorting.
>
> This commit changes DRFSorter to continue to store inactive clients in
> the `clients`; inactive clients are indicated by a new field in the
> `Client` struct, and are omitted from the return value of
> `DRFSorter::sort`.
>
>
> Diffs
> -----
>
> src/master/allocator/sorter/drf/sorter.hpp 76329220e1115c1de7810fb69b943c78c078be59
> src/master/allocator/sorter/drf/sorter.cpp ed54680cecb637931fc344fbcf8fd3b14cc24295
> src/tests/sorter_tests.cpp ec0636beb936d46a253d19322f2157abe95156b6
>
>
> Diff: https://reviews.apache.org/r/57564/diff/2/
>
>
> Testing
> -------
>
> `make check`
>
>
> Thanks,
>
> Neil Conway
>
>
Re: Review Request 57564: Changed DRFSorter's representation of
inactive clients.
Posted by Neil Conway <ne...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57564/
-----------------------------------------------------------
(Updated March 30, 2017, 5:58 p.m.)
Review request for mesos, Benjamin Bannier, Benjamin Mahler, and Michael Park.
Changes
-------
Tweak comments, per review.
Repository: mesos
Description
-------
DRFSorter previously removed inactive clients from the `clients`
collection, and then re-added clients when they were reactivated. This
resulted in resetting the allocation count for the client, which is
unfortunate. This scheme would also be more difficult to adapt to
hierarchical sorting.
This commit changes DRFSorter to continue to store inactive clients in
the `clients`; inactive clients are indicated by a new field in the
`Client` struct, and are omitted from the return value of
`DRFSorter::sort`.
Diffs (updated)
-----
src/master/allocator/sorter/drf/sorter.hpp 76329220e1115c1de7810fb69b943c78c078be59
src/master/allocator/sorter/drf/sorter.cpp ed54680cecb637931fc344fbcf8fd3b14cc24295
src/tests/sorter_tests.cpp ec0636beb936d46a253d19322f2157abe95156b6
Diff: https://reviews.apache.org/r/57564/diff/2/
Changes: https://reviews.apache.org/r/57564/diff/1-2/
Testing
-------
`make check`
Thanks,
Neil Conway