----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/70738/#review215754 -----------------------------------------------------------
Fix it, then Ship it! We've come a long way, the allocation cycle code is a lot more pleasant to read than when we started working on it! src/master/allocator/mesos/hierarchical.cpp Line 1772 (original), 1774 (patched) <https://reviews.apache.org/r/70738/#comment302560> Hm.. I would expect this to say something more like: "In the 1st stage, we allocate to satisfy quota guarantees" The fact that the default guarantees == 0 guarantees -> and therefore no guarantees to satsify seems like an extra mental step for the reader? src/master/allocator/mesos/hierarchical.cpp Lines 1776-1784 (original), 1776-1784 (patched) <https://reviews.apache.org/r/70738/#comment302561> ditto here, the non-default guarantee seems like an extra mental step for the reader? src/master/allocator/mesos/hierarchical.cpp Lines 1796-1797 (patched) <https://reviews.apache.org/r/70738/#comment302562> Hm.. why? src/master/allocator/mesos/hierarchical.cpp Line 1860 (original), 1874 (patched) <https://reviews.apache.org/r/70738/#comment302564> "Rationale": https://writingexplained.org/rational-vs-rationale-difference src/master/allocator/mesos/hierarchical.cpp Line 1874 (original), 1888 (patched) <https://reviews.apache.org/r/70738/#comment302565> "Rationale" src/master/allocator/mesos/hierarchical.cpp Line 1890 (original), 1904 (patched) <https://reviews.apache.org/r/70738/#comment302567> ... to increase a role's quota consumption ("allocate to" seems to fit reservations also, whereas reservations don't increase consumption when allocated) src/master/allocator/mesos/hierarchical.cpp Lines 1930 (patched) <https://reviews.apache.org/r/70738/#comment302568> not sure I understand the comment, "explicitly here" as opposed to? src/master/allocator/mesos/hierarchical.cpp Line 1917 (original), 1942 (patched) <https://reviews.apache.org/r/70738/#comment302569> Can we check against "no limits" rather than default? (Also, just so I understand, this `if` condition is an optimization right?) src/master/allocator/mesos/hierarchical.cpp Line 1925 (original), 1950 (patched) <https://reviews.apache.org/r/70738/#comment302570> This if condition is an optimization right? src/master/allocator/mesos/hierarchical.cpp Lines 1978-1979 (original), 2010-2011 (patched) <https://reviews.apache.org/r/70738/#comment302571> "since allocating these does not make progress towards satsifying quota guarantees"? src/master/allocator/mesos/hierarchical.cpp Lines 2049-2055 (original), 2081-2087 (patched) <https://reviews.apache.org/r/70738/#comment302573> Isn't this just: ``` Resources toAllocate = available.allocatableTo(role).reserved(); ``` No need for the TODO? src/master/allocator/mesos/hierarchical.cpp Line 2064 (original), 2096 (patched) <https://reviews.apache.org/r/70738/#comment302576> Ditto here, can we check against no limits? Ah, I also just realized that this check is needed at the current time because since `rolesConsumedQuota` does not store roles with empty limits. src/master/allocator/mesos/hierarchical.cpp Lines 2145 (patched) <https://reviews.apache.org/r/70738/#comment302577> Fragile to not fill the map with every role, hoping to see this if condition go away soon src/tests/hierarchical_allocator_tests.cpp Line 4425 (original), 4424-4426 (patched) <https://reviews.apache.org/r/70738/#comment302566> s/.get()/->/ - Benjamin Mahler On June 4, 2019, 9:15 p.m., Meng Zhu wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/70738/ > ----------------------------------------------------------- > > (Updated June 4, 2019, 9:15 p.m.) > > > Review request for mesos, Andrei Sekretenko and Benjamin Mahler. > > > Bugs: MESOS-8456 > https://issues.apache.org/jira/browse/MESOS-8456 > > > Repository: mesos > > > Description > ------- > > This patch allows roles to burst above their quota guarantees > up to the quota limits. > > In the first allocation stage which is dedicated to roles > with non-default guarantees, we allow these roles to burst > up to their limits for resources that they have guarantees > set as well as for other resources allocated together > with the guaranteed resources. These bursts are subject to > global headroom enforcement. > > In the second allocation stage, we allow all roles to burst > up to their limits while maintaining the global headroom. > In this stage, if some unreserved non-revocable scalar resources > on the agent need to be held back to maintain the headroom, > we will "chop" them to make the largest offer possible without > breaking the quota headroom. Previously, if any resources are > needed on this agent to maintain the headroom, NO unreserved > scalar resources will be offered at all. > > Also fixed an affected test. > > > Diffs > ----- > > src/master/allocator/mesos/hierarchical.cpp > aa4c43882d0befb2100c92f383e87ab637fa4c7e > src/tests/hierarchical_allocator_tests.cpp > 7221a642a8dda1c7b8ed5ebe7f9b840b5e949e88 > > > Diff: https://reviews.apache.org/r/70738/diff/3/ > > > Testing > ------- > > make check > > > Thanks, > > Meng Zhu > >
