-----------------------------------------------------------
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
> 
>

Reply via email to