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 64003: Made quota resource allocation fine-grained.
Date Thu, 21 Dec 2017 01:06:14 GMT

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




src/master/allocator/mesos/hierarchical.cpp
Lines 1681-1684 (original), 1671-1674 (patched)
<https://reviews.apache.org/r/64003/#comment273001>

    I asked alexr about this TODO, it has become stale because we now have to allocate the
reservations and can't "skip" satisfied roles anymore. Feel free to remove it in a different
patch.



src/master/allocator/mesos/hierarchical.cpp
Line 1686 (original), 1676 (patched)
<https://reviews.apache.org/r/64003/#comment273000>

    is -> is a



src/master/allocator/mesos/hierarchical.cpp
Lines 1741-1752 (original), 1728-1745 (patched)
<https://reviews.apache.org/r/64003/#comment273004>

    Hm.. perhaps a single overall explanation might be more readable? Here's a suggestion
with a TODO for checking headroom:
    
    ```
            // We allocate the role's reservations as well as any unreserved
            // resources while ensuring the role stays within its quota limits.
            // This means that we'll "chop" the unreserved resources up to
            // the quota limit if necessary.
            //
            // E.g. A role has no allocations or reservations yet and a 10 cpu
            //      quota limit. We'll chop a 15 cpu agent down to only
            //      allocate 10 cpus to the role to keep it within its limit.
            //
            // In the case that the role needs some of the resources on this
            // agent to make progress towards its quota, we'll *also* allocate
            // all of the resources for which it does not have quota.
            //
            // E.g. The agent has 1 cpu, 1024 mem, 1024 disk, 1 gpu, 5 ports
            //      and the role has quota for 1 cpu, 1024 mem. We'll include
            //      the disk, gpu, and ports in the allocation, despite the
            //      role not having any quota guarantee for them.
            //
            // We have to do this for now because it's not possible to set
            // quota on non-scalar resources, like ports. However, for scalar
            // resources that can have quota set, we should ideally make
            // sure that when we include them, we're not violating some
            // other role's guarantes!
            //
            // TODO(mzhu): Check the headroom when we're including scalar
            // resources that this role does not have quota for.
            //
            // TODO(mzhu): Since we're treating the resources with unset
            // quota as having no guarantee and no limit, these should be
            // also be allocated further in the second allocation "phase"
            // below (above guarantee up to limit).
    ```



src/master/allocator/mesos/hierarchical.cpp
Line 1752 (original), 1737-1742 (patched)
<https://reviews.apache.org/r/64003/#comment273006>

    Hm.. I'm kind of surprised we have to distinctly track this, isn't this always just equal
to the unreserved non-revocable resources within `resources`?



src/master/allocator/mesos/hierarchical.cpp
Lines 1740-1741 (patched)
<https://reviews.apache.org/r/64003/#comment273007>

    Don't think we need this note, given that it applies to `resources` already? Especially
if we don't need to track this in a separate variable



src/master/allocator/mesos/hierarchical.cpp
Lines 1744-1746 (patched)
<https://reviews.apache.org/r/64003/#comment273008>

    Hm.. this if condition means that if I have:
    
    Quota Guarantee/Limit: 1 cpu, 1024 mem
    Agent has: 1 cpu, 1024 mem, 1 gpu, 5 ports
    
    Case (1):
    Existing Allocation: 1 cpu, 1024 mem
    I'll be allocated: (nothing)
    
    Case (2):
    Existing Allocation: (nothing)
    I'll be allocated: 1 cpu, 1024 mem, 1 gpu, 5 ports
    
    In other words, we are deciding that if you need some resources on this agent to sastisfy
your quota, then we'll give you everything else that you don't have quota for.



src/master/allocator/mesos/hierarchical.cpp
Lines 1747 (patched)
<https://reviews.apache.org/r/64003/#comment273010>

    Updating available seems a little odd? Maybe this should be `Resources unreserved = ...`?



src/master/allocator/mesos/hierarchical.cpp
Lines 1752-1754 (patched)
<https://reviews.apache.org/r/64003/#comment273013>

    Hm.. we may want to shed some light on the disk case that motivated this?
    
    ```
              // When "chopping" resources, there is more than 1 "chop" that
              // can be done to satisfy the limits. Consider the case with
              // two `RAW` disks of 1GB and a "disk" quota of 1GB. We could
              // pick either of the disks here, but not both.
              //
              // In order to avoid repeatedly choosing the same "chop" of
              // the resources each time we allocate, we introduce some
              // randomness by shuffling the resources.
    ```
    
    Not sure if `RAW` is an accurate example, but you get the idea :)



src/master/allocator/mesos/hierarchical.cpp
Lines 1759 (patched)
<https://reviews.apache.org/r/64003/#comment273012>

    Why take a copy here? Should this be `const Resource&`.
    
    I didn't see any mutation, but I think you could do `Resource&` here to mutate in
place if you need to.



src/master/allocator/mesos/hierarchical.cpp
Lines 1760-1763 (patched)
<https://reviews.apache.org/r/64003/#comment273014>

    Might be better to remove this in favor of the comment I suggested above that makes an
overall explanation of the approach



src/master/allocator/mesos/hierarchical.cpp
Lines 1764-1765 (patched)
<https://reviews.apache.org/r/64003/#comment273011>

    Can you do a `.count(resource.name) == 0` instead?



src/master/allocator/mesos/hierarchical.cpp
Lines 1771 (patched)
<https://reviews.apache.org/r/64003/#comment273015>

    CHECK_EQ



src/master/allocator/mesos/hierarchical.cpp
Lines 1773-1776 (patched)
<https://reviews.apache.org/r/64003/#comment273016>

    Hm.. how about `new` or `tentative` instead of `current`?
    
    `.` on same line as `get`



src/master/allocator/mesos/hierarchical.cpp
Lines 1783 (patched)
<https://reviews.apache.org/r/64003/#comment273017>

    const &? Since this isn't returning a temporary



src/master/allocator/mesos/hierarchical.cpp
Lines 1785-1786 (patched)
<https://reviews.apache.org/r/64003/#comment273019>

    Can this fit on one line? I don't think you need the `.value()`s here?
    
    ```
                if (resourceScalar <= newUnsatisfiedQuotaScalar.get()) {
    ```



src/master/allocator/mesos/hierarchical.cpp
Lines 1798 (patched)
<https://reviews.apache.org/r/64003/#comment273020>

    = resource instead of (resource) ? (just might look a bit cleaner)
    
    Also, maybe s/ToAllocate//?


- Benjamin Mahler


On Dec. 20, 2017, 2:06 a.m., Meng Zhu wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64003/
> -----------------------------------------------------------
> 
> (Updated Dec. 20, 2017, 2:06 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Jie Yu, and Michael Park.
> 
> 
> Bugs: MESOS-7099
>     https://issues.apache.org/jira/browse/MESOS-7099
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Allocator now does fine-grained resource allocation up
> to the role’s quota limit. And a role's quota is only
> considered to be satisfied if all of its quota resources
> are satisfied. Previously, a role's quota is considered
> to be met if any one of its quota resources reaches the limit.
> 
> Also fixed a few affected allocator tests.
> 
> 
> Diffs
> -----
> 
>   src/master/allocator/mesos/hierarchical.cpp 1c767f7f778819dcfa6eff51765163aec1b70a2d

>   src/tests/hierarchical_allocator_tests.cpp 173e4fbac184ad8d40c8adba19ad64225f11f1f2

> 
> 
> Diff: https://reviews.apache.org/r/64003/diff/4/
> 
> 
> Testing
> -------
> 
> Fixed a few broken tests due to this patch.
> make check.
> 
> 
> Thanks,
> 
> Meng Zhu
> 
>


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