----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/64698/#review194122 -----------------------------------------------------------
Fix it, then Ship it! Nice fix! I would suggest a summary that makes it clear we're fixing something here, e.g. ``` Fixed a bug where insufficient headroom is held for quota. ``` I think the second paragraph could use a little clarifying as well in terms of required vs available headroom? Or maybe just clarifying how it works? ``` This patch fixes the issue by computing the amount of unreserved resources we need to hold back such that each quota role can have its quota satisfied. Previously, we included the quota role unallocated reservations as part of the headroom since we knew it would be held back, but we didn't handle the case that a quota role had more reservations than quota. ``` Something like this? I can work these minor changes in and get this committed, thanks for the fix Meng! src/master/allocator/mesos/hierarchical.cpp Lines 1838-1844 (patched) <https://reviews.apache.org/r/64698/#comment272856> Hm.. couldn't we do? ``` if (allocated.contains(required)) { continue; // Quota already satisfied. } Resources unallocatedQuota = required - allocated; ``` src/master/allocator/mesos/hierarchical.cpp Lines 1851-1852 (patched) <https://reviews.apache.org/r/64698/#comment272857> I think we can safely avoid the need for checking the subtraction here. - Benjamin Mahler On Dec. 19, 2017, 3:01 a.m., Meng Zhu wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/64698/ > ----------------------------------------------------------- > > (Updated Dec. 19, 2017, 3:01 a.m.) > > > Review request for mesos and Benjamin Mahler. > > > Bugs: MESOS-8339 > https://issues.apache.org/jira/browse/MESOS-8339 > > > Repository: mesos > > > Description > ------- > > If a role has more reservation than its quota, the > current quota headroom calculation is insufficient > in guaranteeing quota allocation. > See MESOS-8339. > > This patch fixes this bug by calculating quota headroom > on a per-role basis (before aggregating) and no longer > counting unallocated reservations (including quota role's > unallocated reservations) towards quota headroom. > > > Diffs > ----- > > src/master/allocator/mesos/hierarchical.cpp > 2cabafeb7ba0cda482ea4464f19730d2ef30cc5e > > > Diff: https://reviews.apache.org/r/64698/diff/1/ > > > Testing > ------- > > make check and a dedicated test #64699 > > > Thanks, > > Meng Zhu > >
