mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Anindya Sinha <anindya_si...@apple.com>
Subject Re: Review Request 57935: Recalculate shares only when total scalar quantities have changed.
Date Fri, 02 Jun 2017 17:00:00 GMT


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


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message