-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67444/#review205762
-----------------------------------------------------------
Looks like a great cleanup!
Persist tends to carry the connotation of writing something to durable storage.
How about:
```
Made quota consumption tracking event-driven in the allocator.
The allocator needs to keep track of role consumed quota
to maintain quota headroom. Currently this info is
constructed on the fly prior to each allocation cycle.
This patch lets the allocator track quota consumption
across allocation cycles in an event-driven manner to improve
performance and reduce code complexity.
```
src/master/allocator/mesos/hierarchical.hpp
Lines 509-512 (patched)
<https://reviews.apache.org/r/67444/#comment288630>
```
// To enforce quota, we keep track of how much quota is consumed by each
role.
// Quota consumption always includes the role's reservations (since they
cannot
// be allocated to other roles) as well as any allocated resources for
the role.
//
// This is only tracked for roles with non-default quota.
```
src/master/allocator/mesos/hierarchical.hpp
Lines 526 (patched)
<https://reviews.apache.org/r/67444/#comment288632>
How about `consumedQuotaScalarQuantities`? We don't call it `rolesQuota`
above for example.
src/master/allocator/mesos/hierarchical.cpp
Lines 1387-1390 (patched)
<https://reviews.apache.org/r/67444/#comment288631>
I don't think we need to repeat ourselves with the "in other words" part. I
think we just need to explain why reservations are included in one place (in
the header):
```
// Track quota consumption.
//
// A role's quota consumption includes its allocation as well as any
// unallocated reservations:
//
// Consumed Quota = reservations + unreserved allocation
// = reservations + allocation - allocated reservations
```
src/master/allocator/mesos/hierarchical.cpp
Lines 1404 (patched)
<https://reviews.apache.org/r/67444/#comment288636>
It wasn't clear to me when reading this why it needs to be done on each
agent, so we should probably clarify that?
```
// Lastly, subtract allocated reservations. This needs to be done on a
per-agent basis because ...
```
src/master/allocator/mesos/hierarchical.cpp
Lines 1405-1406 (patched)
<https://reviews.apache.org/r/67444/#comment288637>
We probably want a TODO to optimize this by avoiding the copy by returning
a const reference to the map?
src/master/allocator/mesos/hierarchical.cpp
Lines 1385-1390 (original), 1413-1418 (patched)
<https://reviews.apache.org/r/67444/#comment288638>
Just an unrelated observation, this note looks misleading, since the master
does try to rescind offers to re-balance.
src/master/allocator/mesos/hierarchical.cpp
Lines 1439-1440 (patched)
<https://reviews.apache.org/r/67444/#comment288639>
It looks like it follows the same order as operations in setQuota, so I
would expect this to be done after metrics.removeQuota. Any reason not to?
Actually, it seems like the metric should get added after quota tracking
and removed before quota tracking is removed, since I would imagine the metrics
look at the tracking information. It looks like we currently only expose
'allocated' instead of 'consumed' for now, but we probably want to expose
'consumed' soon, is there a ticket?
src/master/allocator/mesos/hierarchical.cpp
Lines 2613-2614 (original), 2576-2577 (patched)
<https://reviews.apache.org/r/67444/#comment288640>
Hm.. an aside, but I was puzzled about this function. Since it takes
role->reservations it loses information for non-scalar resources.
Fortunately, this function only uses the scalar quantities anyway, but we
should make the interface clearly take quantities to clarify the assumption.
src/master/allocator/mesos/hierarchical.cpp
Lines 2586-2588 (patched)
<https://reviews.apache.org/r/67444/#comment288642>
It's pretty hard to reason from here about whether this is correct. For
example, how do we know that these quantities were not already tracked because
they were allocated prior to becoming reserved? If that invariant doesn't hold
we'll double count?
src/master/allocator/mesos/hierarchical.cpp
Lines 2607-2611 (patched)
<https://reviews.apache.org/r/67444/#comment288643>
Ditto here, it's hard to reason about why we can remove it here, what if
the resources are unreserved but remain allocated?
src/master/allocator/mesos/hierarchical.cpp
Lines 2727-2728 (patched)
<https://reviews.apache.org/r/67444/#comment288644>
Hm.. there seem to be some invariants here in how the tracking functions
are called but I can't quite figure them out. Is there a way to enforce them?
Let's say I have allocated resoures, these should be tracked. Then I make
them reserved, are they somehow untracked as allocated, the reservations get
tracked, then the updated allocation gets re-tracked? Is that why we skip the
reserved resources here?
We probably need to spell this out more clearly for the reader.
src/master/allocator/mesos/hierarchical.cpp
Lines 2768-2771 (patched)
<https://reviews.apache.org/r/67444/#comment288645>
Ditto here.
- Benjamin Mahler
On June 28, 2018, 8:55 p.m., Meng Zhu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67444/
> -----------------------------------------------------------
>
> (Updated June 28, 2018, 8:55 p.m.)
>
>
> Review request for mesos and Benjamin Mahler.
>
>
> Bugs: MESOS-8802
> https://issues.apache.org/jira/browse/MESOS-8802
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The allocator needs to keep track of role consumed quota
> to maintain quota headroom. Currently this info is
> constructed on the fly prior to each allocation cycle.
>
> This patch lets the allocator buildup and persist this info
> across allocation cycles to improve performance and reduce
> code complexity.
>
>
> Diffs
> -----
>
> src/master/allocator/mesos/hierarchical.hpp
> 0f6c0e96a105c64465d3f5db4ff663d8fdfe7e26
> src/master/allocator/mesos/hierarchical.cpp
> cbdfb2ba9c25755ac631557e0e7dbd721f861a4d
>
>
> Diff: https://reviews.apache.org/r/67444/diff/1/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Meng Zhu
>
>
