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 Sat, 09 Dec 2017 02:08:52 GMT

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



Great to see this getting fixed!

First pass over just the non-testing code. Can you split out the test into a separate patch?


src/master/allocator/mesos/hierarchical.cpp
Lines 1578-1579 (patched)
<https://reviews.apache.org/r/64304/#comment271855>

    s/every allocation/every allocation run/
    
    You might want to clarify that given the other TODO below this one, the plan is to remove
the discrepancy between what the sorter considered allocated and what we'd like to count as
allocated for fair sharing and quota. i.e. we should be able to simplify this and just retrieve
what the role's "allocation" is in the sorter



src/master/allocator/mesos/hierarchical.cpp
Lines 1584-1597 (patched)
<https://reviews.apache.org/r/64304/#comment271856>

    does this need to be a function? Can you just imbed it in your loop below over each quota
role? you also wouldn't need the CHECK anymore?



src/master/allocator/mesos/hierarchical.cpp
Lines 1587-1588 (patched)
<https://reviews.apache.org/r/64304/#comment271857>

    s/&//



src/master/allocator/mesos/hierarchical.cpp
Lines 1587-1588 (patched)
<https://reviews.apache.org/r/64304/#comment271871>

    2 space indent



src/master/allocator/mesos/hierarchical.cpp
Lines 1590 (patched)
<https://reviews.apache.org/r/64304/#comment271858>

    maybe allocatedReservedQuantity?



src/master/allocator/mesos/hierarchical.cpp
Lines 1591 (patched)
<https://reviews.apache.org/r/64304/#comment271859>

    s/(/ (/
    
    .values() will copy (currently), can you use foreachvalue?



src/master/allocator/mesos/hierarchical.cpp
Lines 1592-1593 (patched)
<https://reviews.apache.org/r/64304/#comment271860>

    A note on why we need toUnreserved would be helpful for the reader.



src/master/allocator/mesos/hierarchical.cpp
Lines 1592-1593 (patched)
<https://reviews.apache.org/r/64304/#comment271870>

    2 space indent



src/master/allocator/mesos/hierarchical.cpp
Lines 1599-1601 (patched)
<https://reviews.apache.org/r/64304/#comment271861>

    Let's call it quantities then?
    
    roleAllocatedReservedQuantity
    
    (not sure you need "roles" here)



src/master/allocator/mesos/hierarchical.cpp
Lines 1611 (patched)
<https://reviews.apache.org/r/64304/#comment271862>

    slave



src/master/allocator/mesos/hierarchical.cpp
Lines 1618-1621 (patched)
<https://reviews.apache.org/r/64304/#comment271846>

    s/name/role/ and 2 space indents



src/master/allocator/mesos/hierarchical.cpp
Lines 1602-1609 (original), 1649-1667 (patched)
<https://reviews.apache.org/r/64304/#comment271869>

    2 space indent



src/master/allocator/mesos/hierarchical.cpp
Lines 1663 (patched)
<https://reviews.apache.org/r/64304/#comment271851>

    What goes wrong without this being fixed? Is it fixed later in your chain? Seems like
you wrote a test for it?



src/master/allocator/mesos/hierarchical.cpp
Lines 1666-1667 (patched)
<https://reviews.apache.org/r/64304/#comment271866>

    How about (x - y) + z?



src/master/allocator/mesos/hierarchical.cpp
Lines 1673 (patched)
<https://reviews.apache.org/r/64304/#comment271867>

    unreserved



src/master/allocator/mesos/hierarchical.cpp
Lines 1808 (patched)
<https://reviews.apache.org/r/64304/#comment271872>

    2 space indent



src/master/allocator/mesos/hierarchical.cpp
Lines 1808-1810 (patched)
<https://reviews.apache.org/r/64304/#comment271876>

    you can take the slaveId by reference to avoid copying it?



src/master/allocator/mesos/hierarchical.cpp
Lines 1811-1813 (patched)
<https://reviews.apache.org/r/64304/#comment271874>

    rolesAllocatedReservedResources[role] +=
              (resources.reserved(role).nonShared() + newShared)
                .createStrippedScalarQuantity().toUnreserved();



src/master/allocator/mesos/hierarchical.cpp
Lines 1813 (patched)
<https://reviews.apache.org/r/64304/#comment271875>

    a comment here as well about why we need toUnreserved would be helpful


- Benjamin Mahler


On Dec. 8, 2017, 9:51 p.m., Meng Zhu wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64304/
> -----------------------------------------------------------
> 
> (Updated Dec. 8, 2017, 9:51 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 9d8b6714d060f67d570c5653798e705248781869

>   src/tests/hierarchical_allocator_tests.cpp 862f4683da04d37d9fe9f471d6ec9cd7751f39ec

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


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