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