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 64304: Enforced quota limit in the presence of unallocated reservations.
Date Tue, 05 Dec 2017 01:08:23 GMT

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




src/master/allocator/mesos/hierarchical.cpp
Line 1492 (original), 1492 (patched)
<https://reviews.apache.org/r/64304/#comment271070>

    Feel free to send a small patch for this since it's an indpendent change, I can get it
committed quickly.



src/master/allocator/mesos/hierarchical.cpp
Line 2351 (original), 2370 (patched)
<https://reviews.apache.org/r/64304/#comment271069>

    Whoops! Can you put this in the original patch?



src/tests/hierarchical_allocator_tests.cpp
Lines 1270-1271 (patched)
<https://reviews.apache.org/r/64304/#comment271046>

    Reservations should be accounted towards the quota guarantee/limit even if they are currently
unallocated.



src/tests/hierarchical_allocator_tests.cpp
Lines 1272 (patched)
<https://reviews.apache.org/r/64304/#comment271049>

    After reading through the test, this is the dynamic reservation case only? Should we call
this test `QuotaLimitWithDynamicReservation` and have another test for static reservations?
Or do you want to test both cases in this test?



src/tests/hierarchical_allocator_tests.cpp
Lines 1310-1311 (patched)
<https://reviews.apache.org/r/64304/#comment271052>

    This note seems to contradict the test? Is it a copy paste mistake?



src/tests/hierarchical_allocator_tests.cpp
Lines 1321 (patched)
<https://reviews.apache.org/r/64304/#comment271053>

    As a general comment, be sure to start comments with a capital letter and end them with
a period, please do a sweep across this patch for other instances. There are also some typos
in the comments, so be sure to do a self-review before publishing a patch.



src/tests/hierarchical_allocator_tests.cpp
Lines 1330-1331 (patched)
<https://reviews.apache.org/r/64304/#comment271054>

    This comment seems like a copy/paste mistake? I'm not sure you need a settle here.



src/tests/hierarchical_allocator_tests.cpp
Lines 1347-1348 (patched)
<https://reviews.apache.org/r/64304/#comment271055>

    allocated to the role



src/tests/hierarchical_allocator_tests.cpp
Lines 1355 (patched)
<https://reviews.apache.org/r/64304/#comment271056>

    What is a satisfied reservation? An allocated reservation?



src/tests/hierarchical_allocator_tests.cpp
Lines 1358 (patched)
<https://reviews.apache.org/r/64304/#comment271058>

    I think you mean allocated resources + unallocated reservations?



src/tests/hierarchical_allocator_tests.cpp
Lines 1359 (patched)
<https://reviews.apache.org/r/64304/#comment271059>

    the quota limit



src/tests/hierarchical_allocator_tests.cpp
Lines 1359-1361 (patched)
<https://reviews.apache.org/r/64304/#comment271060>

    These last two sentences were a bit confusing, I think you can do without them if you
update the addition above to reflect the lack of double counting?



src/tests/hierarchical_allocator_tests.cpp
Lines 1362 (patched)
<https://reviews.apache.org/r/64304/#comment271061>

    Looks like this is already stated at the top, can you remove this?



src/tests/hierarchical_allocator_tests.cpp
Lines 1363 (patched)
<https://reviews.apache.org/r/64304/#comment271062>

    This test is also just dynamic reservations, so the same comment above applies. Maybe
we need to parameterize these tests based on whether to use static or dynamic reservation?
    
    It's also not clear to me why we need both tests, instead of just a single test.
    
    E.g.
    
    quota = R
    add agent 1 with R resources, reserve them, decline forever
    add agent 2 with R resources, they should not be allocated
    
    revive
    
    should only get agent 1 resources offered
    trigger another allocation cycle (to make sure it's enforced across cycles)
    agent 2 should not be offered to the role



src/tests/hierarchical_allocator_tests.cpp
Lines 1401 (patched)
<https://reviews.apache.org/r/64304/#comment271064>

    Ditto, this looks like a copy/paste mistake?



src/tests/hierarchical_allocator_tests.cpp
Lines 4737-4796 (original), 4902-4956 (patched)
<https://reviews.apache.org/r/64304/#comment271063>

    Can you pull this bit out into a separate patch? We can get this committed independently.



src/master/allocator/mesos/hierarchical.cpp
Lines 1543-1556 (patched)
<https://reviews.apache.org/r/64304/#comment271082>

    I think ultimately, the sorter needs to consider reserved resources as "allocated", do
you have any TODO related to this or any tickets?
    
    I guess the "unfairly satisfy guarantees" in [MESOS-4527](https://issues.apache.org/jira/browse/MESOS-4527)
means that we couldn't close this ticket until we also track the reservations in the sorters,
which will affect the code here (i.e. you might not need to track the reservations separately?)
    
    Also, we would need a ticket for reservations breaking fairness for the non-quota roles,
I don't think there is one yet.



src/master/allocator/mesos/hierarchical.cpp
Lines 1569-1572 (original), 1588-1594 (patched)
<https://reviews.apache.org/r/64304/#comment271083>

    I'm weary of introducing new terminology here (i.e. "possession" of resources). Maybe
`resourcesChargedAgainstQuota`?



src/master/allocator/mesos/hierarchical.cpp
Lines 1645-1653 (original), 1673-1681 (patched)
<https://reviews.apache.org/r/64304/#comment271081>

    Can you add a note here about [MESOS-7099](https://issues.apache.org/jira/browse/MESOS-7099)?



src/master/allocator/mesos/hierarchical.cpp
Lines 1797-1798 (patched)
<https://reviews.apache.org/r/64304/#comment271080>

    How about:
    
    ```
        // TODO(mzhu): Breaking early here means that we may leave reservations
        // unallocated. See MESOS-8293.
    ```


- Benjamin Mahler


On Dec. 4, 2017, 11:18 p.m., Meng Zhu wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64304/
> -----------------------------------------------------------
> 
> (Updated Dec. 4, 2017, 11:18 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Benjamin Mahler, and Michael Park.
> 
> 
> Bugs: MESOS-4527
>     https://issues.apache.org/jira/browse/MESOS-4527
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Enforced quota limit in the presence of unallocated reservations.
> Also modify the allocation process such that the first stage allocation
> is solely for quota roles while the second stage is solely for
> non-quota roles.
> 
> 
> Diffs
> -----
> 
>   src/master/allocator/mesos/hierarchical.cpp 20b908cc891f9f9be3045cd9f8be83a11d37ab78

>   src/tests/hierarchical_allocator_tests.cpp 0309074bab180be122c9b0074981e6f69c97feee

> 
> 
> Diff: https://reviews.apache.org/r/64304/diff/3/
> 
> 
> Testing
> -------
> 
> Introduced two dedicated tests.
> 
> 
> Thanks,
> 
> Meng Zhu
> 
>


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