> On May 1, 2018, 12:39 a.m., Benjamin Mahler wrote: > > src/master/allocator/mesos/hierarchical.cpp > > Lines 1766 (patched) > > <https://reviews.apache.org/r/66861/diff/1/?file=2014553#file2014553line1766> > > > > Do we need the typedef up here? or should we put it down to the foreach > > loop to be clear it's needed there? (if that's why it was added)
Good call, makes more sense to move it down. > On May 1, 2018, 12:39 a.m., Benjamin Mahler wrote: > > src/master/allocator/mesos/hierarchical.cpp > > Lines 1775-1790 (patched) > > <https://reviews.apache.org/r/66861/diff/1/?file=2014553#file2014553line1775> > > > > Maybe something like this? > > > > ``` > > auto frameworks = &drfPositions[role]; > > auto framework = frameworks.find(frameworkId); > > > > if (framework == frameworks.end()) { > > framework = frameworks.emplace(frameworkId, {position, > > position}).second; > > } > > > > framework.first = std::min(framework.first, position); > > framework.second = std::max(framework.second, position); > > ``` > > > > Or: > > > > ``` > > auto frameworks = &drfPositions[role]; > > auto framework = frameworks.find(frameworkId); > > > > if (framework == frameworks.end()) { > > frameworks.emplace(frameworkId, {position, position}); > > } else { > > framework.first = std::min(framework.first, position); > > framework.second = std::max(framework.second, position); > > } > > ``` > > > > This also avoids most of the excessive map lookups while also making it > > more readable? Definite improvement; thanks! > On May 1, 2018, 12:39 a.m., Benjamin Mahler wrote: > > src/master/allocator/mesos/hierarchical.cpp > > Lines 1968-1970 (original), 2005-2007 (patched) > > <https://reviews.apache.org/r/66861/diff/1/?file=2014553#file2014553line2005> > > > > We break the framework id loop here, which I think screws up the min / > > max calculation? Ideally we would also break the role loop if nothing is > > allocatable, but we don't do that today (hopefully we could make it easy to > > add the latter breaking without the metrics getting screwed up?) > > > > Ditto below. Thanks for catching this. I elected to store the sorted frameworks in a local variable and increment metrics immediately, so we should be fine in the future regardless of what kind of breaking we decide to do. - Greg ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/66861/#review202169 ----------------------------------------------------------- On May 8, 2018, 10:18 p.m., Greg Mann wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/66861/ > ----------------------------------------------------------- > > (Updated May 8, 2018, 10:18 p.m.) > > > Review request for mesos, Benjamin Mahler, Gaston Kleiman, Gilbert Song, and > Vinod Kone. > > > Repository: mesos > > > Description > ------- > > During each allocation cycle, the allocator re-sorts roles and > frameworks for each agent in the cluster. This means that for each > agent there exists a total order of (role, framework) tuples. > > This patch adds per-framework, per-role metrics which track the > minimum and maximum positions attained by the framework in this > sorting process, from the most recent allocation cycle. > > > Diffs > ----- > > src/master/allocator/mesos/hierarchical.cpp > 1000968be6a2935a4cac571414d7f06d7df7acf0 > src/master/allocator/mesos/metrics.hpp > 6d386225c301d5ab44f3cc0ecdd1478fb5162e5b > src/master/allocator/mesos/metrics.cpp > 5194f5b3b3f507b36f02300775230157db3dd477 > > > Diff: https://reviews.apache.org/r/66861/diff/2/ > > > Testing > ------- > > > Thanks, > > Greg Mann > >
