----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/43879/#review124937 -----------------------------------------------------------
Fix it, then Ship it! Thanks! Just some minor adjustments I'll make before committing. docs/monitoring.md (line 873) <https://reviews.apache.org/r/43879/#comment187639> How about: ``` Number of times the allocation algorithm has run ``` src/master/allocator/mesos/metrics.hpp (line 59) <https://reviews.apache.org/r/43879/#comment187641> How about: // Number of times the allocation algorithm has run. src/tests/hierarchical_allocator_tests.cpp (lines 2684 - 2686) <https://reviews.apache.org/r/43879/#comment187643> Could we put this next to the resource metrics test? src/tests/hierarchical_allocator_tests.cpp (lines 2684 - 2685) <https://reviews.apache.org/r/43879/#comment187645> How about: ``` // This test checks that the number of times the allocation // algorithm has run is correctly reflected in the metric. ``` src/tests/hierarchical_allocator_tests.cpp (line 2686) <https://reviews.apache.org/r/43879/#comment187644> s/Metrics/Metric/ in this patch since there's only one src/tests/hierarchical_allocator_tests.cpp (line 2695) <https://reviews.apache.org/r/43879/#comment187646> We'll typically use size_t for this kind of variable src/tests/hierarchical_allocator_tests.cpp (line 2699) <https://reviews.apache.org/r/43879/#comment187647> It's a bit unfortunate to use the [] operator here since it will insert the value if it was missing, could we switch to .contains? This would be consistent with the comments I made on the previous patches. Also, why not use your allocations variable? s/0/allocations/ - Ben Mahler On March 21, 2016, 11:08 a.m., Benjamin Bannier wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/43879/ > ----------------------------------------------------------- > > (Updated March 21, 2016, 11:08 a.m.) > > > Review request for mesos, Alexander Rukletsov and Ben Mahler. > > > Bugs: MESOS-4718 > https://issues.apache.org/jira/browse/MESOS-4718 > > > Repository: mesos > > > Description > ------- > > Added allocator metrics for number of allocations made. > > > Diffs > ----- > > docs/monitoring.md 827f7073204fcf8575ca980a5571c8be4f5e4110 > src/master/allocator/mesos/hierarchical.cpp > 70291075c00a9a557529c2562dedcfc6c6c3ec32 > src/master/allocator/mesos/metrics.hpp > d04e9a11c77b6fb2d522608e3085f81f8a6657ca > src/master/allocator/mesos/metrics.cpp > 46dd7f7b3d8d49c59b42e789fe5dcd49f1693ace > src/tests/hierarchical_allocator_tests.cpp > 459e02576f6d05abbbcc83ae5cabac5c66e93f05 > > Diff: https://reviews.apache.org/r/43879/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 > >
