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 64698: Made quota headroom calculation on a per-role basis.
Date Tue, 19 Dec 2017 05:14:24 GMT

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


Fix it, then Ship it!




Nice fix!

I would suggest a summary that makes it clear we're fixing something here, e.g.

```
Fixed a bug where insufficient headroom is held for quota.
```

I think the second paragraph could use a little clarifying as well in terms of required vs
available headroom? Or maybe just clarifying how it works?

```
This patch fixes the issue by computing the amount of unreserved
resources we need to hold back such that each quota role can have
its quota satisfied. Previously, we included the quota role
unallocated reservations as part of the headroom since we knew it
would be held back, but we didn't handle the case that a quota role
had more reservations than quota.
```

Something like this?

I can work these minor changes in and get this committed, thanks for the fix Meng!


src/master/allocator/mesos/hierarchical.cpp
Lines 1838-1844 (patched)
<https://reviews.apache.org/r/64698/#comment272856>

    Hm.. couldn't we do?
    
    ```
        if (allocated.contains(required)) {
          continue; // Quota already satisfied.
        }
        
        Resources unallocatedQuota = required - allocated;
    ```



src/master/allocator/mesos/hierarchical.cpp
Lines 1851-1852 (patched)
<https://reviews.apache.org/r/64698/#comment272857>

    I think we can safely avoid the need for checking the subtraction here.


- Benjamin Mahler


On Dec. 19, 2017, 3:01 a.m., Meng Zhu wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64698/
> -----------------------------------------------------------
> 
> (Updated Dec. 19, 2017, 3:01 a.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Bugs: MESOS-8339
>     https://issues.apache.org/jira/browse/MESOS-8339
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> If a role has more reservation than its quota, the
> current quota headroom calculation is insufficient
> in guaranteeing quota allocation.
> See MESOS-8339.
> 
> This patch fixes this bug by calculating quota headroom
> on a per-role basis (before aggregating) and no longer
> counting unallocated reservations (including quota role's
> unallocated reservations) towards quota headroom.
> 
> 
> Diffs
> -----
> 
>   src/master/allocator/mesos/hierarchical.cpp 2cabafeb7ba0cda482ea4464f19730d2ef30cc5e

> 
> 
> Diff: https://reviews.apache.org/r/64698/diff/1/
> 
> 
> Testing
> -------
> 
> make check and a dedicated test #64699
> 
> 
> Thanks,
> 
> Meng Zhu
> 
>


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