> On April 5, 2019, 12:59 a.m., Meng Zhu wrote: > > src/master/allocator/mesos/hierarchical.cpp > > Lines 1714-1715 (patched) > > <https://reviews.apache.org/r/70393/diff/2/?file=2137549#file2137549line1714> > > > > I think this could just be: > > > > ``` > > const string& topLevelRole = role.substr(0, role.find('/')); > > ``` > > > > I do not have strong opinion on this, I will leave this up to you.
Hm.. intresting, it's a bit "clever" but also, it means that in the typical case today of the role being already top level, we would do an unnecessary copy (potential malloc) vs the current approach in the patch which only copies if non-top level? (of course, the additional scan and branch have costs too.. :)). I think since the reader reasons about it as two cases (non top level and top level), it reads a bit easier with the condition. > On April 5, 2019, 12:59 a.m., Meng Zhu wrote: > > src/master/allocator/mesos/hierarchical.cpp > > Lines 1711-1725 (original), 1722-1736 (patched) > > <https://reviews.apache.org/r/70393/diff/2/?file=2137549#file2137549line1722> > > > > We should be able to combine these two? > > > > ``` > > // Then add allocated unreserved resource quantities. > > if (roleSorter->contains(role)) { > > foreachvalue (const Resources& resources, > > roleSorter->allocation(role)) { > > rolesConsumedQuota[topLevelRole] += > > ResourceQuantities::fromScalarResources(resources.unreserved().nonRevocable().scalars()); > > } > > } > > > > ``` Yes, good point and nice and succict! This means I should probably adjust the formula above to reflect that we now just use the first form rather than the expanded form with the subtraction. - Benjamin ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/70393/#review214393 ----------------------------------------------------------- On April 4, 2019, 11 p.m., Benjamin Mahler wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/70393/ > ----------------------------------------------------------- > > (Updated April 4, 2019, 11 p.m.) > > > Review request for mesos and Meng Zhu. > > > Bugs: MESOS-9688 and MESOS-9691 > https://issues.apache.org/jira/browse/MESOS-9688 > https://issues.apache.org/jira/browse/MESOS-9691 > > > Repository: mesos > > > Description > ------- > > There was an invalid assumption that the sorter returns > hierarchically aggregated allocation information, whereas > it actually returns the "." internal node information. This > patch adjusts the allocator code accordingly. > > > Diffs > ----- > > src/master/allocator/mesos/hierarchical.cpp > 1fd8b021c9954c37533dc193b3148def5ff53071 > > > Diff: https://reviews.apache.org/r/70393/diff/2/ > > > Testing > ------- > > make check > > > Thanks, > > Benjamin Mahler > >
