> On March 4, 2016, 2:05 p.m., Alexander Rukletsov wrote: > > src/master/allocator/mesos/hierarchical.cpp, lines 275-280 > > <https://reviews.apache.org/r/43881/diff/15/?file=1279203#file1279203line275> > > > > I would pull this into a helper in `Metrics` similar to > > `createGaugesForResource()`. This way we keep the allocator code minimal.
Done. > On March 4, 2016, 2:05 p.m., Alexander Rukletsov wrote: > > src/master/allocator/mesos/hierarchical.cpp, lines 344-347 > > <https://reviews.apache.org/r/43881/diff/15/?file=1279203#file1279203line344> > > > > Maybe here a helper as well. Done. > On March 4, 2016, 2:05 p.m., Alexander Rukletsov wrote: > > src/master/allocator/mesos/hierarchical.cpp, lines 1498-1499 > > <https://reviews.apache.org/r/43881/diff/15/?file=1279203#file1279203line1498> > > > > I think this should be around `allocated()` calls for sorters. When > > `allocated()` is called, the sorter's internal allocation counter is > > incremented. I'd say we should keep the counters in sync. > > > > Alternatively, we can expose the allocations counter from sorters and > > query it directly. For now I put these in the locations where a `Sorter`'s `allocated` is called. > On March 4, 2016, 2:05 p.m., Alexander Rukletsov wrote: > > src/master/allocator/mesos/metrics.cpp, lines 59-60 > > <https://reviews.apache.org/r/43881/diff/15/?file=1279205#file1279205line59> > > > > If you say `using process::metrics::Counter` at the top, you can fit > > this into one line : ). Done, also for other `Metric`s in introduced here in this series. > On March 4, 2016, 2:05 p.m., Alexander Rukletsov wrote: > > src/tests/hierarchical_allocator_tests.cpp, line 2464 > > <https://reviews.apache.org/r/43881/diff/15/?file=1279206#file1279206line2464> > > > > Backtick `framework1` please. Done. > On March 4, 2016, 2:05 p.m., Alexander Rukletsov wrote: > > src/tests/hierarchical_allocator_tests.cpp, lines 2464-2472 > > <https://reviews.apache.org/r/43881/diff/15/?file=1279206#file1279206line2464> > > > > Do you want to set quota as part of your test? Because otherwise we can > > simplify the test a bit by not setting it and having `agent2` allocated to > > `framework2`. It does not hurt introducing it here, and we will make more explicit use of this information at a later point in this series. > On March 4, 2016, 2:05 p.m., Alexander Rukletsov wrote: > > src/tests/hierarchical_allocator_tests.cpp, line 2487 > > <https://reviews.apache.org/r/43881/diff/15/?file=1279206#file1279206line2487> > > > > Feel free to kill this line It's dead, Alex :D > On March 4, 2016, 2:05 p.m., Alexander Rukletsov wrote: > > src/tests/hierarchical_allocator_tests.cpp, lines 2492-2495 > > <https://reviews.apache.org/r/43881/diff/15/?file=1279206#file1279206line2492> > > > > How about > > ``` > > Clock::advance(flags.allocation_interval); > > Clock::settle(); > > ++allocations; > > ``` > > to keep clock manipulation together? So be it. - Benjamin ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/43881/#review122053 ----------------------------------------------------------- On March 4, 2016, 5:01 p.m., Benjamin Bannier wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/43881/ > ----------------------------------------------------------- > > (Updated March 4, 2016, 5:01 p.m.) > > > Review request for mesos, Alexander Rukletsov and Ben Mahler. > > > Bugs: MESOS-4719 > https://issues.apache.org/jira/browse/MESOS-4719 > > > Repository: mesos > > > Description > ------- > > See summary. > > > Diffs > ----- > > docs/monitoring.md 827f7073204fcf8575ca980a5571c8be4f5e4110 > src/master/allocator/mesos/hierarchical.cpp > 70291075c00a9a557529c2562dedcfc6c6c3ec32 > src/master/allocator/mesos/metrics.hpp PRE-CREATION > src/master/allocator/mesos/metrics.cpp PRE-CREATION > src/tests/hierarchical_allocator_tests.cpp > 3e4ad31925e1b815a74d67fa3962d23fa5bc89d1 > > Diff: https://reviews.apache.org/r/43881/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 > >
