> On May 24, 2019, 4:57 p.m., Benjamin Mahler wrote: > > src/master/allocator/mesos/hierarchical.hpp > > Line 123 (original), 125-126 (patched) > > <https://reviews.apache.org/r/70707/diff/2/?file=2146621#file2146621line127> > > > > just one on each line at this point seems more readable?
Sounds good. > On May 24, 2019, 4:57 p.m., Benjamin Mahler wrote: > > src/master/allocator/mesos/hierarchical.hpp > > Lines 644-645 (patched) > > <https://reviews.apache.org/r/70707/diff/2/?file=2146621#file2146621line651> > > > > "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()) { > > ... > > } > > ``` > "Has quota" seems inaccurate I could name it `hasQuotaSet` > will break down soon when we introduce the ability to set the limits why? We could just need to add the limit check as well return roles.contains(role) && (!roles.at(role).quotaGuarantees.empty() || ! roles.at(role).quotaLimits.empty()) > Can't this be a property of the role struct? It certainly could. But (1) it is very verbose and hurts readability; (2) See my answer below. > On May 24, 2019, 4:57 p.m., Benjamin Mahler wrote: > > src/master/allocator/mesos/hierarchical.cpp > > Lines 2558 (patched) > > <https://reviews.apache.org/r/70707/diff/2/?file=2146622#file2146622line2561> > > > > Per my above comment, when would roles not contain role when we're > > asking about whether a role has quota? This depends on "when we're asking" :) While most of the time a role struct would exist, it is not always true and this is what makes the check necessary. Just a couple of examples: - https://github.com/apache/mesos/blob/master/src/master/allocator/mesos/hierarchical.cpp#L1436 Change this line to `CHECK(!roleHasQuota(role));` Here, a role may not exist if the quota info is all it has atm. - https://github.com/apache/mesos/blob/264a27ae1c9f7fc587c3c7daf60025c11c98305f/src/master/allocator/mesos/hierarchical.cpp#L1731 I think top-level roles may not exist despite the existence of sub-level roles I think without a complete role life cycle management, it is really hard to say when a role exists or not. Until we add life cycle management, I suggest let's do the existence check for the hasQuotaSet call. What do you think? - Meng ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/70707/#review215518 ----------------------------------------------------------- On May 24, 2019, 4: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, 4: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 > >
