mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Benjamin Mahler <bmah...@apache.org>
Subject Re: Review Request 67444: Persisted role consumed quota info in the allocator.
Date Thu, 05 Jul 2018 21:22:09 GMT

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


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