> 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()) { > > ... > > } > > ``` > > Meng Zhu wrote: > > "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.
Introduced a helper `getGuarantees()` to look up quota guarantees of a given role, it would return an empty ResourceQuantity object if the role only has default guarantees. - Meng ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/70707/#review215518 ----------------------------------------------------------- On May 29, 2019, 8:22 a.m., Meng Zhu wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/70707/ > ----------------------------------------------------------- > > (Updated May 29, 2019, 8:22 a.m.) > > > Review request for mesos, Andrei Sekretenko and Benjamin Mahler. > > > Repository: mesos > > > Description > ------- > > This avoids an extra map of tracking role quota info. > Also added a helper `getGuarantees()` to look up quota guarantees > of a given role. > > > Diffs > ----- > > src/master/allocator/mesos/hierarchical.hpp > 71a9656fb934bf9ac58e3165254ea49cb09efa8b > src/master/allocator/mesos/hierarchical.cpp > 40c8363afddccdd5275ca06318a8cc2cc6fa21af > > > Diff: https://reviews.apache.org/r/70707/diff/4/ > > > Testing > ------- > > make check > > > Thanks, > > Meng Zhu > >
