mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Meng Zhu <m...@mesosphere.io>
Subject Re: Review Request 67444: Made quota consumption tracking event-driven in the allocator.
Date Tue, 31 Jul 2018 23:57:09 GMT


> On July 5, 2018, 2:22 p.m., Benjamin Mahler wrote:
> > 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.
> > ```

Sounds good. Thanks!


> On July 5, 2018, 2:22 p.m., Benjamin Mahler wrote:
> > src/master/allocator/mesos/hierarchical.cpp
> > Lines 1404 (patched)
> > <https://reviews.apache.org/r/67444/diff/1/?file=2046973#file2046973line1404>
> >
> >     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 ...
> >     ```

I do not think we have anything interesting to say here. We subtract on a per-agent basis
because currently there is no aggregated bookkeeping info. In the future, it is possible for
us to track aggregated allocated reservations and subtract all at once. Dropping.


> On July 5, 2018, 2:22 p.m., Benjamin Mahler wrote:
> > src/master/allocator/mesos/hierarchical.cpp
> > Lines 1405-1406 (patched)
> > <https://reviews.apache.org/r/67444/diff/1/?file=2046973#file2046973line1405>
> >
> >     We probably want a TODO to optimize this by avoiding the copy by returning a
const reference to the map?

Looks like the `quotaRoleSorter->allocation(role)` is already returning a const ref, must
be a slip earlier. Will update these in an immediate followup patch.


> On July 5, 2018, 2:22 p.m., Benjamin Mahler wrote:
> > src/master/allocator/mesos/hierarchical.cpp
> > Lines 1385-1390 (original), 1413-1418 (patched)
> > <https://reviews.apache.org/r/67444/diff/1/?file=2046973#file2046973line1413>
> >
> >     Just an unrelated observation, this note looks misleading, since the master
does try to rescind offers to re-balance.

"Re-balancing" means a change of allocation? Anyway, added a todo here:

  // TODO(mzhu): Re-evaluate the above assumption and consider triggering an
  // allocation here.


> On July 5, 2018, 2:22 p.m., Benjamin Mahler wrote:
> > src/master/allocator/mesos/hierarchical.cpp
> > Lines 1439-1440 (patched)
> > <https://reviews.apache.org/r/67444/diff/1/?file=2046973#file2046973line1439>
> >
> >     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?

Filed MESOS-9123 for exposing role consumed quota.

Updated `metrics.setQuota` to be done after the tracking info update. But for `metrics.removeQuota`,
it does not depend on any of the tracking info since it is all about removing the metric entries.
I think it can be done either before or after the tracking info update. For consistency, let's
update the metrics after the tracking info update in both add and remove.


> On July 5, 2018, 2:22 p.m., Benjamin Mahler wrote:
> > src/master/allocator/mesos/hierarchical.cpp
> > Lines 2586-2588 (patched)
> > <https://reviews.apache.org/r/67444/diff/1/?file=2046973#file2046973line2654>
> >
> >     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?
> 
> Meng Zhu wrote:
>     Thanks for catching this!
>     
>     The invariant should be:
>     (1) when tracking reservation, only track unallocated reservations
>     (2) when track allocation, only track unreserved allocations
>     
>     (2) is already being done. (1) is hard to do given the current abstraction, we will
need more than raw "reservations". Will need more refactoring on the Slave struct in the allocator
to make this work and readable. Will come back to this later.
>     
>     In addition, this reminds me that we do not have tests for the above scenario yet.
Will add some.

Now fixed.


- Meng


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67444/#review205762
-----------------------------------------------------------


On July 31, 2018, 4:56 p.m., Meng Zhu wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67444/
> -----------------------------------------------------------
> 
> (Updated July 31, 2018, 4:56 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 track quota consumption
> across allocation cycles in an event-driven manner to improve
> performance and reduce code complexity.
> 
> 
> Diffs
> -----
> 
>   src/master/allocator/mesos/hierarchical.hpp 0cd2fac17f09110c46b8540523a9c935f156f858

>   src/master/allocator/mesos/hierarchical.cpp 35992474eacb8b14ae57e1dc23307e1542f63cb5

> 
> 
> Diff: https://reviews.apache.org/r/67444/diff/2/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Meng Zhu
> 
>


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