----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/64636/#review193993 -----------------------------------------------------------
Fix it, then Ship it! Perhaps the required / available headroom computations should be contained in functions? ``` Resources requiredQuotaHeadroom() const; Resources availableQuotaHeadroom() const; ``` I guess it's hard to then define how to grow/shrink the headroom since the definition of what consists of headroom is spread between the function and the allocation loop that's trying to increment it instead of re-compute it? src/master/allocator/mesos/hierarchical.cpp Lines 1826 (patched) <https://reviews.apache.org/r/64636/#comment272686> s/name/role/ src/master/allocator/mesos/hierarchical.cpp Lines 1833 (patched) <https://reviews.apache.org/r/64636/#comment272685> Perhaps an = here? src/master/allocator/mesos/hierarchical.cpp Lines 1852-1854 (original), 1878-1880 (patched) <https://reviews.apache.org/r/64636/#comment272692> What's this for? src/master/allocator/mesos/hierarchical.cpp Lines 2020-2021 (patched) <https://reviews.apache.org/r/64636/#comment272696> This seemed a little backwards when I read it, I was expecting to see something more like: available headroom - what we're going to allocate >= required headroom So: ``` (availableHeadroom - headroomToAllocate).contains(requiredHeadroom) ``` src/master/allocator/mesos/hierarchical.cpp Lines 2032 (patched) <https://reviews.apache.org/r/64636/#comment272697> This should be unstripped? Looks like we need to strip for the headroom calculation but not here? - Benjamin Mahler On Dec. 15, 2017, 2:34 a.m., Meng Zhu wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/64636/ > ----------------------------------------------------------- > > (Updated Dec. 15, 2017, 2:34 a.m.) > > > Review request for mesos and Benjamin Mahler. > > > Bugs: MESOS-8293 > https://issues.apache.org/jira/browse/MESOS-8293 > > > Repository: mesos > > > Description > ------- > > Also fixed a bug where quota headroom may wrongly include revocable > resources. > > > Diffs > ----- > > src/master/allocator/mesos/hierarchical.cpp > fccd71c96fe8e4d914e19b5ea8d8f69e9ebf2406 > > > Diff: https://reviews.apache.org/r/64636/diff/2/ > > > Testing > ------- > > make check. > > > Thanks, > > Meng Zhu > >
