----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/70707/#review215518 -----------------------------------------------------------
src/master/allocator/mesos/hierarchical.hpp Line 123 (original), 125-126 (patched) <https://reviews.apache.org/r/70707/#comment302252> just one on each line at this point seems more readable? src/master/allocator/mesos/hierarchical.hpp Lines 644-645 (patched) <https://reviews.apache.org/r/70707/#comment302253> "Has quota" seems inaccurate and will break down soon when we introduce the ability to set the limits. Strictly speaking, every role has a quota, it just might be 0 guarantee / no limit (default). Can't this be a property of the role struct? Looking at code like this: ``` if (roleHasQuota(role)) { ``` Makes me think the code should just use the role struct: ``` Role r = roles.at(role); if (r.quota.guarantee > ResourceQuantities()) { ... } ``` src/master/allocator/mesos/hierarchical.cpp Lines 2558 (patched) <https://reviews.apache.org/r/70707/#comment302254> Per my above comment, when would roles not contain role when we're asking about whether a role has quota? - Benjamin Mahler On May 24, 2019, 11:52 a.m., Meng Zhu wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/70707/ > ----------------------------------------------------------- > > (Updated May 24, 2019, 11:52 a.m.) > > > Review request for mesos, Andrei Sekretenko and Benjamin Mahler. > > > Repository: mesos > > > Description > ------- > > This avoids an extra map of tracking role quota info. > > > Diffs > ----- > > src/master/allocator/mesos/hierarchical.hpp > 7e4f30d16010a777ff29ba35366ae80ff2631322 > src/master/allocator/mesos/hierarchical.cpp > ccca391fbf73d9aa9c31f7552e39fb78c242e242 > > > Diff: https://reviews.apache.org/r/70707/diff/2/ > > > Testing > ------- > > make check > > > Thanks, > > Meng Zhu > >
