> 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 > >
