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 64467: Rewrote the quota headroom enforcement logic in the allocator.
Date Tue, 12 Dec 2017 05:03:49 GMT

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



Can you also describe how it worked before in the description? I think that would be helpful
for the reader and for posterity.


src/master/allocator/mesos/hierarchical.cpp
Lines 1834-1840 (patched)
<https://reviews.apache.org/r/64467/#comment272071>

    I found this code and the the stuff above it a little hard to follow, maybe you could
have a comment about the formula at the top and then have the code for computing it?



src/master/allocator/mesos/hierarchical.cpp
Line 1873 (original), 1853 (patched)
<https://reviews.apache.org/r/64467/#comment272062>

    Hm.. rather than naming this based on what we use it for, we should probably name it based
on the condition it represents?
    
    E.g.
    
    ```
    bool headroomSatisfied = ...;
    
    if (!headroomSatisfied) {
      // allocate only reservations
    } else {
      // allocate all
    }
    ```



src/master/allocator/mesos/hierarchical.cpp
Lines 1861 (patched)
<https://reviews.apache.org/r/64467/#comment272069>

    Why are you removing shared?



src/master/allocator/mesos/hierarchical.cpp
Lines 1866 (patched)
<https://reviews.apache.org/r/64467/#comment272070>

    Hm.. shouldn't this be a quantity? Otherwise you're comparing a full set of resources
against a quantity?
    
    Probably you want some tests to have agents with non-scalar resources (e.g. "ports") to
expose this issue?



src/master/allocator/mesos/hierarchical.cpp
Lines 1868-1872 (patched)
<https://reviews.apache.org/r/64467/#comment272066>

    Does this need to be in the if condition above? Seems like it would be an optimization
below it. Also, it seems a little brittle that available is declared twice, maybe we should
update the same variable?
    
    ```
    bool headroomSatisfied;
    ...
    
    foreach (const SlaveID& slaveId, slaveIds) {
       ...
    
      if (!headroomSatisfied) {
        headroomSatisfied = currentHeadRoom.contains(requiredHeadRoom);
      }
      
      // Until the headroom is satisfied, we hold back the unreserved
      // resources and only offer the reservations.
      Resources available = headroomSatisfied ?
        slave.available() :
        slave.available().reserved();
      
      currentHeadRoom += slave.available().unreserved().toScalarQuantity();
      
      // As an optimization, we skip the agent early if the headroom
      // is not satisfied and there are no reservations to allocate.
      if (!headroomSatisfied && available.reserved().empty()) {
        continue;
      }
      
      foreach (const string& role, roleSorter->sort()) {
         ...
         // allocate
         available -= resources
         ...
      }
    ```



src/master/allocator/mesos/hierarchical.cpp
Lines 1914-1923 (original), 1910-1918 (patched)
<https://reviews.apache.org/r/64467/#comment272067>

    This seems a little distracting from the diff, it looks like an independent cleanup? Could
you pull it out so I can get it committed more quickly and we can have a cleaner diff for
this change?



src/master/allocator/mesos/hierarchical.cpp
Lines 1931-1939 (original), 1926-1932 (patched)
<https://reviews.apache.org/r/64467/#comment272068>

    Hm.. doesn't the ancestor comment still apply? Your change is altering the sematics?
    
    It should be the case that "eng/frontend" would get offered a reservation for "eng", but
that won't happen if the headroom is not satisfied? It would only go to "eng/frontend" directly
in that case?



src/tests/hierarchical_allocator_tests.cpp
Lines 3398-3405 (original), 3391-3403 (patched)
<https://reviews.apache.org/r/64467/#comment272061>

    Do you need the non-determinism here?


- Benjamin Mahler


On Dec. 9, 2017, 12:26 a.m., Meng Zhu wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64467/
> -----------------------------------------------------------
> 
> (Updated Dec. 9, 2017, 12:26 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Michael Park.
> 
> 
> Bugs: MESOS-8293
>     https://issues.apache.org/jira/browse/MESOS-8293
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Now before offering unreserved resources to frameworks, the
> resources are holdout for the quota headroom until the headroom
> is met (reserved resources are offered unaffected).
> 
> 
> Diffs
> -----
> 
>   src/master/allocator/mesos/hierarchical.cpp 9d8b6714d060f67d570c5653798e705248781869

>   src/tests/hierarchical_allocator_tests.cpp 862f4683da04d37d9fe9f471d6ec9cd7751f39ec

> 
> 
> Diff: https://reviews.apache.org/r/64467/diff/3/
> 
> 
> Testing
> -------
> 
> make check and a dediated test in #64465
> 
> 
> Thanks,
> 
> Meng Zhu
> 
>


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