You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Anindya Sinha <an...@apple.com> on 2017/03/26 01:16:15 UTC
Review Request 57935: Added a check to ensure scalar quantities match
in `DRFSorter::update`.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57935/
-----------------------------------------------------------
Review request for mesos and Neil Conway.
Bugs: MESOS-7138
https://issues.apache.org/jira/browse/MESOS-7138
Repository: mesos
Description
-------
The old and new allocations only varies in roles (i.e. reservations), or
`DiskInfo` (for persistent volumes) or shared counts. In these cases, we
do not re-calculate the shares since the total allocation quantity is
unchanged.
Diffs
-----
src/master/allocator/sorter/drf/sorter.cpp ed54680cecb637931fc344fbcf8fd3b14cc24295
Diff: https://reviews.apache.org/r/57935/diff/1/
Testing
-------
All tests passed.
Thanks,
Anindya Sinha
Re: Review Request 57935: Added a check to ensure scalar quantities
match in `DRFSorter::update`.
Posted by Mesos Reviewbot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57935/#review170106
-----------------------------------------------------------
Patch looks great!
Reviews applied: [57935]
Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' CONFIGURATION='--verbose' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker-build.sh
- Mesos Reviewbot
On March 26, 2017, 1:16 a.m., Anindya Sinha wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57935/
> -----------------------------------------------------------
>
> (Updated March 26, 2017, 1:16 a.m.)
>
>
> Review request for mesos and Neil Conway.
>
>
> Bugs: MESOS-7138
> https://issues.apache.org/jira/browse/MESOS-7138
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The old and new allocations only varies in roles (i.e. reservations), or
> `DiskInfo` (for persistent volumes) or shared counts. In these cases, we
> do not re-calculate the shares since the total allocation quantity is
> unchanged.
>
>
> Diffs
> -----
>
> src/master/allocator/sorter/drf/sorter.cpp ed54680cecb637931fc344fbcf8fd3b14cc24295
>
>
> Diff: https://reviews.apache.org/r/57935/diff/1/
>
>
> Testing
> -------
>
> All tests passed.
>
>
> Thanks,
>
> Anindya Sinha
>
>
Re: Review Request 57935: Recalculate shares only when total scalar
quantities have changed.
Posted by Mesos Reviewbot Windows <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57935/#review176824
-----------------------------------------------------------
Patch looks great!
Reviews applied: [57935]
Passed command: support\windows-build.bat
- Mesos Reviewbot Windows
On June 2, 2017, 4:59 p.m., Anindya Sinha wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57935/
> -----------------------------------------------------------
>
> (Updated June 2, 2017, 4:59 p.m.)
>
>
> Review request for mesos and Neil Conway.
>
>
> Bugs: MESOS-7138
> https://issues.apache.org/jira/browse/MESOS-7138
>
>
> Repository: mesos
>
>
> Description
> -------
>
> In `DRFSorter::update`, we should set the `dirty` flag only when the
> total scalar quantities have changed in any of the clients in the
> hierarchy, and not always.
>
>
> Diffs
> -----
>
> src/master/allocator/sorter/drf/sorter.hpp 77e52dec735d276389643f7f356cd763b2f785e9
> src/master/allocator/sorter/drf/sorter.cpp ecc5586737b6b447c5a1cf1a37037832bcbacd69
>
>
> Diff: https://reviews.apache.org/r/57935/diff/4/
>
>
> Testing
> -------
>
> All tests passed.
>
>
> Thanks,
>
> Anindya Sinha
>
>
Re: Review Request 57935: Recalculate shares only when total scalar
quantities have changed.
Posted by Anindya Sinha <an...@apple.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57935/
-----------------------------------------------------------
(Updated June 2, 2017, 4:59 p.m.)
Review request for mesos and Neil Conway.
Changes
-------
Addressed review comments.
Bugs: MESOS-7138
https://issues.apache.org/jira/browse/MESOS-7138
Repository: mesos
Description (updated)
-------
In `DRFSorter::update`, we should set the `dirty` flag only when the
total scalar quantities have changed in any of the clients in the
hierarchy, and not always.
Diffs (updated)
-----
src/master/allocator/sorter/drf/sorter.hpp 77e52dec735d276389643f7f356cd763b2f785e9
src/master/allocator/sorter/drf/sorter.cpp ecc5586737b6b447c5a1cf1a37037832bcbacd69
Diff: https://reviews.apache.org/r/57935/diff/4/
Changes: https://reviews.apache.org/r/57935/diff/3-4/
Testing
-------
All tests passed.
Thanks,
Anindya Sinha
Re: Review Request 57935: Recalculate shares only when total scalar
quantities have changed.
Posted by Anindya Sinha <an...@apple.com>.
> On June 2, 2017, 4:26 p.m., Neil Conway wrote:
> > src/master/allocator/sorter/drf/sorter.hpp
> > Lines 347 (patched)
> > <https://reviews.apache.org/r/57935/diff/3/?file=1716055#file1716055line347>
> >
> > Why is `flatten` necessary here? We are already comparing two scalar quantities, so is the flatten required?
In the case of a `RESERVE` or `UNRESERVE` operation, the `newAllocation` and `oldAllocation` varies in distribution of resources across roles.
eg. For `RESERVE`, oldAllocation = `disk(*):100`, and newAllocation = `disk(*):90; disk(role1):10`.
So, `createStrippedScalarQuantities()` on these `Resources` shall drop the `ReservationInfo` but the roles will remain intact.
Without `flatten()`, `oldAllocationQuantity` and `newAllocationQuantity` will not be the same (due to different roles) and hence `dirty` shall be set. But I do not think we need to recalculate shares since the total resources have not changed (only the distribution has changed in terms of roles). That is the reason for the comparison having the `flatten()`.
Looking at when `dirty` is true: We update `totals` (which is hashmap `<ResourceName, ScalarValue>` in `update()`. And when `calculateShare()` is called, we calculate share based on totals in the Sorter and individual Node. So I think we should be good to have the `flatten()` in this comparison.
Let me know if this does not sound ok.
> On June 2, 2017, 4:26 p.m., Neil Conway wrote:
> > src/master/allocator/sorter/drf/sorter.cpp
> > Lines 307 (patched)
> > <https://reviews.apache.org/r/57935/diff/3/?file=1716056#file1716056line312>
> >
> >
Dropping this as there is no comment here.
> On June 2, 2017, 4:26 p.m., Neil Conway wrote:
> > src/master/allocator/sorter/drf/sorter.cpp
> > Lines 307 (patched)
> > <https://reviews.apache.org/r/57935/diff/3/?file=1716056#file1716056line312>
> >
> >
Dropping this as there is no comment here.
> On June 2, 2017, 4:26 p.m., Neil Conway wrote:
> > src/master/allocator/sorter/drf/sorter.cpp
> > Lines 307 (patched)
> > <https://reviews.apache.org/r/57935/diff/3/?file=1716056#file1716056line312>
> >
> > The return value of `update` depends only on the parameters `oldAllocation` and `newAllocation`, right? In which case, the way this is written seems confusing to me (e.g., it implies it would be possible for `dirty` to remain false when we start at a leaf node, but then for `dirty` to be true for an ancestor node).
> >
> > An improvement would be to `CHECK` that `update()` returns the same value for all the nodes in the path from leaf -> root.
> >
> > We could alternatively check whether the total allocation has changed outside `update` and only update `dirty` once. We could conceivably compute `oldAllocationQuantity` and `newAllocationQuantity` in `allocated` and pass them into `update` (for efficiency), but that is a bit ugly.
> >
> > Let me know what you think.
SGTM. I modified based on the 1st recommendation. I do a `CHECK` to ensure that `update()` returns the same value for all nodes in the path from leaf -> root.
- Anindya
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57935/#review176687
-----------------------------------------------------------
On June 2, 2017, 4:59 p.m., Anindya Sinha wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57935/
> -----------------------------------------------------------
>
> (Updated June 2, 2017, 4:59 p.m.)
>
>
> Review request for mesos and Neil Conway.
>
>
> Bugs: MESOS-7138
> https://issues.apache.org/jira/browse/MESOS-7138
>
>
> Repository: mesos
>
>
> Description
> -------
>
> In `DRFSorter::update`, we should set the `dirty` flag only when the
> total scalar quantities have changed in any of the clients in the
> hierarchy, and not always.
>
>
> Diffs
> -----
>
> src/master/allocator/sorter/drf/sorter.hpp 77e52dec735d276389643f7f356cd763b2f785e9
> src/master/allocator/sorter/drf/sorter.cpp ecc5586737b6b447c5a1cf1a37037832bcbacd69
>
>
> Diff: https://reviews.apache.org/r/57935/diff/4/
>
>
> Testing
> -------
>
> All tests passed.
>
>
> Thanks,
>
> Anindya Sinha
>
>
Re: Review Request 57935: Recalculate shares only when total scalar
quantities have changed.
Posted by Anindya Sinha <an...@apple.com>.
> On June 2, 2017, 4:26 p.m., Neil Conway wrote:
> > src/master/allocator/sorter/drf/sorter.hpp
> > Lines 347 (patched)
> > <https://reviews.apache.org/r/57935/diff/3/?file=1716055#file1716055line347>
> >
> > Why is `flatten` necessary here? We are already comparing two scalar quantities, so is the flatten required?
>
> Anindya Sinha wrote:
> In the case of a `RESERVE` or `UNRESERVE` operation, the `newAllocation` and `oldAllocation` varies in distribution of resources across roles.
>
> eg. For `RESERVE`, oldAllocation = `disk(*):100`, and newAllocation = `disk(*):90; disk(role1):10`.
>
> So, `createStrippedScalarQuantities()` on these `Resources` shall drop the `ReservationInfo` but the roles will remain intact.
> Without `flatten()`, `oldAllocationQuantity` and `newAllocationQuantity` will not be the same (due to different roles) and hence `dirty` shall be set. But I do not think we need to recalculate shares since the total resources have not changed (only the distribution has changed in terms of roles). That is the reason for the comparison having the `flatten()`.
>
> Looking at when `dirty` is true: We update `totals` (which is hashmap `<ResourceName, ScalarValue>` in `update()`. And when `calculateShare()` is called, we calculate share based on totals in the Sorter and individual Node. So I think we should be good to have the `flatten()` in this comparison.
>
> Let me know if this does not sound ok.
>
> Neil Conway wrote:
> Sounds reasonable.
So, can this be merged now?
- Anindya
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57935/#review176687
-----------------------------------------------------------
On June 2, 2017, 4:59 p.m., Anindya Sinha wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57935/
> -----------------------------------------------------------
>
> (Updated June 2, 2017, 4:59 p.m.)
>
>
> Review request for mesos and Neil Conway.
>
>
> Bugs: MESOS-7138
> https://issues.apache.org/jira/browse/MESOS-7138
>
>
> Repository: mesos
>
>
> Description
> -------
>
> In `DRFSorter::update`, we should set the `dirty` flag only when the
> total scalar quantities have changed in any of the clients in the
> hierarchy, and not always.
>
>
> Diffs
> -----
>
> src/master/allocator/sorter/drf/sorter.hpp 77e52dec735d276389643f7f356cd763b2f785e9
> src/master/allocator/sorter/drf/sorter.cpp ecc5586737b6b447c5a1cf1a37037832bcbacd69
>
>
> Diff: https://reviews.apache.org/r/57935/diff/4/
>
>
> Testing
> -------
>
> All tests passed.
>
>
> Thanks,
>
> Anindya Sinha
>
>
Re: Review Request 57935: Recalculate shares only when total scalar
quantities have changed.
Posted by Neil Conway <ne...@gmail.com>.
> On June 2, 2017, 4:26 p.m., Neil Conway wrote:
> > src/master/allocator/sorter/drf/sorter.hpp
> > Lines 347 (patched)
> > <https://reviews.apache.org/r/57935/diff/3/?file=1716055#file1716055line347>
> >
> > Why is `flatten` necessary here? We are already comparing two scalar quantities, so is the flatten required?
>
> Anindya Sinha wrote:
> In the case of a `RESERVE` or `UNRESERVE` operation, the `newAllocation` and `oldAllocation` varies in distribution of resources across roles.
>
> eg. For `RESERVE`, oldAllocation = `disk(*):100`, and newAllocation = `disk(*):90; disk(role1):10`.
>
> So, `createStrippedScalarQuantities()` on these `Resources` shall drop the `ReservationInfo` but the roles will remain intact.
> Without `flatten()`, `oldAllocationQuantity` and `newAllocationQuantity` will not be the same (due to different roles) and hence `dirty` shall be set. But I do not think we need to recalculate shares since the total resources have not changed (only the distribution has changed in terms of roles). That is the reason for the comparison having the `flatten()`.
>
> Looking at when `dirty` is true: We update `totals` (which is hashmap `<ResourceName, ScalarValue>` in `update()`. And when `calculateShare()` is called, we calculate share based on totals in the Sorter and individual Node. So I think we should be good to have the `flatten()` in this comparison.
>
> Let me know if this does not sound ok.
>
> Neil Conway wrote:
> Sounds reasonable.
>
> Anindya Sinha wrote:
> So, can this be merged now?
Sorry, should have updated the ticket with the progress here. I wanted to get BenM's input on the best way to do the update for multiple levels of the hierarchy. He's tied up this week but we're hoping to get some time to discuss on Monday -- would be great to loop you in via hangouts or slack.
- Neil
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57935/#review176687
-----------------------------------------------------------
On June 2, 2017, 4:59 p.m., Anindya Sinha wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57935/
> -----------------------------------------------------------
>
> (Updated June 2, 2017, 4:59 p.m.)
>
>
> Review request for mesos and Neil Conway.
>
>
> Bugs: MESOS-7138
> https://issues.apache.org/jira/browse/MESOS-7138
>
>
> Repository: mesos
>
>
> Description
> -------
>
> In `DRFSorter::update`, we should set the `dirty` flag only when the
> total scalar quantities have changed in any of the clients in the
> hierarchy, and not always.
>
>
> Diffs
> -----
>
> src/master/allocator/sorter/drf/sorter.hpp 77e52dec735d276389643f7f356cd763b2f785e9
> src/master/allocator/sorter/drf/sorter.cpp ecc5586737b6b447c5a1cf1a37037832bcbacd69
>
>
> Diff: https://reviews.apache.org/r/57935/diff/4/
>
>
> Testing
> -------
>
> All tests passed.
>
>
> Thanks,
>
> Anindya Sinha
>
>
Re: Review Request 57935: Recalculate shares only when total scalar
quantities have changed.
Posted by Neil Conway <ne...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57935/#review176687
-----------------------------------------------------------
src/master/allocator/sorter/drf/sorter.hpp
Lines 346 (patched)
<https://reviews.apache.org/r/57935/#comment250116>
I think `totalChanged` would be a more idiomatic variable name.
src/master/allocator/sorter/drf/sorter.hpp
Lines 347 (patched)
<https://reviews.apache.org/r/57935/#comment250122>
Why is `flatten` necessary here? We are already comparing two scalar quantities, so is the flatten required?
src/master/allocator/sorter/drf/sorter.hpp
Lines 358 (patched)
<https://reviews.apache.org/r/57935/#comment250123>
I'd move the definition/calculation of `isTotalChanged` down here, to where it is used.
src/master/allocator/sorter/drf/sorter.cpp
Line 309 (original), 304 (patched)
<https://reviews.apache.org/r/57935/#comment250127>
src/master/allocator/sorter/drf/sorter.cpp
Lines 307 (patched)
<https://reviews.apache.org/r/57935/#comment250124>
src/master/allocator/sorter/drf/sorter.cpp
Lines 307 (patched)
<https://reviews.apache.org/r/57935/#comment250125>
src/master/allocator/sorter/drf/sorter.cpp
Lines 307 (patched)
<https://reviews.apache.org/r/57935/#comment250126>
The return value of `update` depends only on the parameters `oldAllocation` and `newAllocation`, right? In which case, the way this is written seems confusing to me (e.g., it implies it would be possible for `dirty` to remain false when we start at a leaf node, but then for `dirty` to be true for an ancestor node).
An improvement would be to `CHECK` that `update()` returns the same value for all the nodes in the path from leaf -> root.
We could alternatively check whether the total allocation has changed outside `update` and only update `dirty` once. We could conceivably compute `oldAllocationQuantity` and `newAllocationQuantity` in `allocated` and pass them into `update` (for efficiency), but that is a bit ugly.
Let me know what you think.
- Neil Conway
On May 11, 2017, 9:02 p.m., Anindya Sinha wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57935/
> -----------------------------------------------------------
>
> (Updated May 11, 2017, 9:02 p.m.)
>
>
> Review request for mesos and Neil Conway.
>
>
> Bugs: MESOS-7138
> https://issues.apache.org/jira/browse/MESOS-7138
>
>
> Repository: mesos
>
>
> Description
> -------
>
> In `DRFSorter::update`, we should set the `dirty` flag only when the
> total scalar quantities have changed in any of the cleints in the
> hierarchy, and not always.
>
>
> Diffs
> -----
>
> src/master/allocator/sorter/drf/sorter.hpp fee58d6d1f08163e2a06a4a20c891fe535c3dcff
> src/master/allocator/sorter/drf/sorter.cpp 26b77f578f3235a8792c72d4575d607cdb2c7de7
>
>
> Diff: https://reviews.apache.org/r/57935/diff/3/
>
>
> Testing
> -------
>
> All tests passed.
>
>
> Thanks,
>
> Anindya Sinha
>
>
Re: Review Request 57935: Recalculate shares only when total scalar
quantities have changed.
Posted by Mesos Reviewbot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57935/#review174794
-----------------------------------------------------------
Patch looks great!
Reviews applied: [57935]
Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' CONFIGURATION='--verbose' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker-build.sh
- Mesos Reviewbot
On May 11, 2017, 2:02 p.m., Anindya Sinha wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57935/
> -----------------------------------------------------------
>
> (Updated May 11, 2017, 2:02 p.m.)
>
>
> Review request for mesos and Neil Conway.
>
>
> Bugs: MESOS-7138
> https://issues.apache.org/jira/browse/MESOS-7138
>
>
> Repository: mesos
>
>
> Description
> -------
>
> In `DRFSorter::update`, we should set the `dirty` flag only when the
> total scalar quantities have changed in any of the cleints in the
> hierarchy, and not always.
>
>
> Diffs
> -----
>
> src/master/allocator/sorter/drf/sorter.hpp fee58d6d1f08163e2a06a4a20c891fe535c3dcff
> src/master/allocator/sorter/drf/sorter.cpp 26b77f578f3235a8792c72d4575d607cdb2c7de7
>
>
> Diff: https://reviews.apache.org/r/57935/diff/3/
>
>
> Testing
> -------
>
> All tests passed.
>
>
> Thanks,
>
> Anindya Sinha
>
>
Re: Review Request 57935: Recalculate shares only when total scalar
quantities have changed.
Posted by Anindya Sinha <an...@apple.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57935/
-----------------------------------------------------------
(Updated May 11, 2017, 9:02 p.m.)
Review request for mesos and Neil Conway.
Changes
-------
Rebased.
Bugs: MESOS-7138
https://issues.apache.org/jira/browse/MESOS-7138
Repository: mesos
Description
-------
In `DRFSorter::update`, we should set the `dirty` flag only when the
total scalar quantities have changed in any of the cleints in the
hierarchy, and not always.
Diffs (updated)
-----
src/master/allocator/sorter/drf/sorter.hpp fee58d6d1f08163e2a06a4a20c891fe535c3dcff
src/master/allocator/sorter/drf/sorter.cpp 26b77f578f3235a8792c72d4575d607cdb2c7de7
Diff: https://reviews.apache.org/r/57935/diff/3/
Changes: https://reviews.apache.org/r/57935/diff/2-3/
Testing
-------
All tests passed.
Thanks,
Anindya Sinha
Re: Review Request 57935: Recalculate shares only when total scalar
quantities have changed.
Posted by Mesos Reviewbot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57935/#review173283
-----------------------------------------------------------
Patch looks great!
Reviews applied: [57935]
Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' CONFIGURATION='--verbose' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker-build.sh
- Mesos Reviewbot
On April 27, 2017, 5:35 p.m., Anindya Sinha wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57935/
> -----------------------------------------------------------
>
> (Updated April 27, 2017, 5:35 p.m.)
>
>
> Review request for mesos and Neil Conway.
>
>
> Bugs: MESOS-7138
> https://issues.apache.org/jira/browse/MESOS-7138
>
>
> Repository: mesos
>
>
> Description
> -------
>
> In `DRFSorter::update`, we should set the `dirty` flag only when the
> total scalar quantities have changed in any of the cleints in the
> hierarchy, and not always.
>
>
> Diffs
> -----
>
> src/master/allocator/sorter/drf/sorter.hpp fee58d6d1f08163e2a06a4a20c891fe535c3dcff
> src/master/allocator/sorter/drf/sorter.cpp a8b35c4deac7a4f0725ccae517a8cbca5b8e1ee7
>
>
> Diff: https://reviews.apache.org/r/57935/diff/2/
>
>
> Testing
> -------
>
> All tests passed.
>
>
> Thanks,
>
> Anindya Sinha
>
>
Re: Review Request 57935: Recalculate shares only when total scalar
quantities have changed.
Posted by Anindya Sinha <an...@apple.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57935/
-----------------------------------------------------------
(Updated April 27, 2017, 5:35 p.m.)
Review request for mesos and Neil Conway.
Changes
-------
Rebased on top of changes in hierarchical roles implementation.
Summary (updated)
-----------------
Recalculate shares only when total scalar quantities have changed.
Bugs: MESOS-7138
https://issues.apache.org/jira/browse/MESOS-7138
Repository: mesos
Description (updated)
-------
In `DRFSorter::update`, we should set the `dirty` flag only when the
total scalar quantities have changed in any of the cleints in the
hierarchy, and not always.
Diffs (updated)
-----
src/master/allocator/sorter/drf/sorter.hpp fee58d6d1f08163e2a06a4a20c891fe535c3dcff
src/master/allocator/sorter/drf/sorter.cpp a8b35c4deac7a4f0725ccae517a8cbca5b8e1ee7
Diff: https://reviews.apache.org/r/57935/diff/2/
Changes: https://reviews.apache.org/r/57935/diff/1-2/
Testing
-------
All tests passed.
Thanks,
Anindya Sinha
Re: Review Request 57935: Recalculate shares only when total scalar
quantities have changed.
Posted by Anindya Sinha <an...@apple.com>.
> On April 26, 2017, 7:26 p.m., Neil Conway wrote:
> > Hey Anindya -- can you rebase this RR for the recent sorter changes? I should be able to review and commit it shortly after that.
Rebased this patch.
btw, `SorterTest.HierarchicalAllocation()` has the following call to `sorter.update()`:
```
sorter.update("b/c", slaveId, cResources, cNewResources);
```
which updates the sorter from `cResources` of `cpus(*):4;mem(*):4` to `cNewResources` of `cpus(*):1;mem(*):1`. This obviously changes the `totals` and hence the `dirty` flag needs to be set.
However, `DRFSorter::update()` is only called from `HierarchicalAllocatorProcess::updateAllocation()` for each of the role sorters and IIUC, there should not be a case when the `totals` would actually change. So, I was of the opinion of putting in a check invariant in `Node::Allocation::update()` but obviously that would be a problem for this test. So, as of now, I added a check that if `totals` changes, we set the `dirty` flag.
- Anindya
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57935/#review173081
-----------------------------------------------------------
On April 27, 2017, 5:35 p.m., Anindya Sinha wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57935/
> -----------------------------------------------------------
>
> (Updated April 27, 2017, 5:35 p.m.)
>
>
> Review request for mesos and Neil Conway.
>
>
> Bugs: MESOS-7138
> https://issues.apache.org/jira/browse/MESOS-7138
>
>
> Repository: mesos
>
>
> Description
> -------
>
> In `DRFSorter::update`, we should set the `dirty` flag only when the
> total scalar quantities have changed in any of the cleints in the
> hierarchy, and not always.
>
>
> Diffs
> -----
>
> src/master/allocator/sorter/drf/sorter.hpp fee58d6d1f08163e2a06a4a20c891fe535c3dcff
> src/master/allocator/sorter/drf/sorter.cpp a8b35c4deac7a4f0725ccae517a8cbca5b8e1ee7
>
>
> Diff: https://reviews.apache.org/r/57935/diff/2/
>
>
> Testing
> -------
>
> All tests passed.
>
>
> Thanks,
>
> Anindya Sinha
>
>
Re: Review Request 57935: Added a check to ensure scalar quantities
match in `DRFSorter::update`.
Posted by Neil Conway <ne...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57935/#review173081
-----------------------------------------------------------
Hey Anindya -- can you rebase this RR for the recent sorter changes? I should be able to review and commit it shortly after that.
- Neil Conway
On March 26, 2017, 1:16 a.m., Anindya Sinha wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57935/
> -----------------------------------------------------------
>
> (Updated March 26, 2017, 1:16 a.m.)
>
>
> Review request for mesos and Neil Conway.
>
>
> Bugs: MESOS-7138
> https://issues.apache.org/jira/browse/MESOS-7138
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The old and new allocations only varies in roles (i.e. reservations), or
> `DiskInfo` (for persistent volumes) or shared counts. In these cases, we
> do not re-calculate the shares since the total allocation quantity is
> unchanged.
>
>
> Diffs
> -----
>
> src/master/allocator/sorter/drf/sorter.cpp ed54680cecb637931fc344fbcf8fd3b14cc24295
>
>
> Diff: https://reviews.apache.org/r/57935/diff/1/
>
>
> Testing
> -------
>
> All tests passed.
>
>
> Thanks,
>
> Anindya Sinha
>
>