----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/43883/#review125485 -----------------------------------------------------------
The code looks pretty clean, thanks! However, there were two bugs here: (1) The value of the metric only counts how many slaves have offer filters, rather than the total number of filters (note the value type of `offerFilters` is a set). (2) removeRole was not removing the role from the map of gauges I made some minor adjustments based on the comments here and fixed these two bugs, will commit shortly! docs/monitoring.md (line 955) <https://reviews.apache.org/r/43883/#comment188276> In the interest of consistency and avoiding repeating ourselves in the name, perhaps the following? ``` allocator/mesos/filters/roles/<role>/active ``` src/master/allocator/mesos/hierarchical.hpp (line 298) <https://reviews.apache.org/r/43883/#comment188273> Hm.. why the "role" naming prefix here but no "role" naming prefix on `_quota_allocated`? src/master/allocator/mesos/hierarchical.cpp (line 1742) <https://reviews.apache.org/r/43883/#comment188275> 2 spaces here src/master/allocator/mesos/hierarchical.cpp (line 1743) <https://reviews.apache.org/r/43883/#comment188281> How about wrapping the argument on the next line here for consistency? src/master/allocator/mesos/hierarchical.cpp (line 1747) <https://reviews.apache.org/r/43883/#comment188277> whoops, extra newline? src/master/allocator/mesos/hierarchical.cpp (line 1753) <https://reviews.apache.org/r/43883/#comment188278> Whoops, this looks like a bug. `offerFilters` is a `hashmap<SlaveID, hashset<OfferFilter*>>` and so it looks like we need to loop over the values and use the sum of the sizes, since there may be more than one filter per slave: ``` foreachkey (const SlaveID& slaveId, framework.offerFilters) { result += framework.offerFilters[slaveId].size(); } ``` src/master/allocator/mesos/metrics.hpp (lines 51 - 52) <https://reviews.apache.org/r/43883/#comment188274> Very nice! src/master/allocator/mesos/metrics.cpp (line 113) <https://reviews.apache.org/r/43883/#comment188272> s/(/ (/ src/master/allocator/mesos/metrics.cpp (line 175) <https://reviews.apache.org/r/43883/#comment188279> `offer_filter` here but `gauge` in removeRole? src/master/allocator/mesos/metrics.cpp (line 185) <https://reviews.apache.org/r/43883/#comment188271> 2 spaces here src/master/allocator/mesos/metrics.cpp (lines 188 - 192) <https://reviews.apache.org/r/43883/#comment188285> This looks like a bug: we also have to remove the role from the map here. src/tests/hierarchical_allocator_tests.cpp (line 568) <https://reviews.apache.org/r/43883/#comment188280> Since the metric is declared higher up it's a bit tricker to follow what is being checked here, how about s/metric/filters_active/ ? src/tests/hierarchical_allocator_tests.cpp (lines 2655 - 2657) <https://reviews.apache.org/r/43883/#comment188283> Thanks for the test! src/tests/hierarchical_allocator_tests.cpp (lines 2700 - 2707) <https://reviews.apache.org/r/43883/#comment188282> Some newlines here would be nice: ``` Future<Allocation> allocation = allocations.get(); AWAIT_READY(allocation); ASSERT_EQ(framework1.id(), allocation->frameworkId); allocator->recoverResources( allocation->frameworkId, agent.id(), allocation->resources.get(agent.id()).get(), offerFilter); ``` - Ben Mahler On March 24, 2016, 1:36 p.m., Benjamin Bannier wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/43883/ > ----------------------------------------------------------- > > (Updated March 24, 2016, 1:36 p.m.) > > > Review request for mesos, Alexander Rukletsov and Ben Mahler. > > > Bugs: MESOS-4722 > https://issues.apache.org/jira/browse/MESOS-4722 > > > Repository: mesos > > > Description > ------- > > Added a metric for querying the number offer filters for a role. > > > Diffs > ----- > > docs/monitoring.md dcf19e6ce06b02373a2bd1af81a451a35743fa76 > src/master/allocator/mesos/hierarchical.hpp > e4604f4da59166da24709a68b8cd4e56bf55f97f > src/master/allocator/mesos/hierarchical.cpp > 39a290d0db2c22e179a8f933b1a78e3a2dcefdc3 > src/master/allocator/mesos/metrics.hpp > b5f2806cff99ee2ee46c4ac8a13174ef699969aa > src/master/allocator/mesos/metrics.cpp > 2f3012a5bd40a4100e790e1d373832015c80b993 > src/tests/hierarchical_allocator_tests.cpp > e9cfcfc0ad8b0b89bbac459b7db39183f6c189be > > Diff: https://reviews.apache.org/r/43883/diff/ > > > Testing > ------- > > make check (OS X) > > I confirmed that this does not lead to general performance regressions in the > allocator; this is partially expected since the added code only inserts > metrics in the allocator while the actual work is perform asynchronously. > These tests where performed with > `HierarchicalAllocator_BENCHMARK_Test.DeclineOffers` on an optimized build > under OS X using clang(trunk) as compiler. > > > Thanks, > > Benjamin Bannier > >
