----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/43880/#review124864 -----------------------------------------------------------
Fix it, then Ship it! Thanks for the patience Benjamin! I'll make some adjustments based on the comments below, and get this committed shortly. docs/monitoring.md (lines 869 - 917) <https://reviews.apache.org/r/43880/#comment187560> Let's say a user makes a custom resource named "quota", then it mixes with the "quota" directory we created earlier. The suggestion from the last review was to add a "resources/" prefix, sound good? Once we follow up on ensuring that all resources are represented (not just cpus, mem, disk), it would also be great to change this to just be parameterized on <resource> like we did with quota. docs/monitoring.md (line 887) <https://reviews.apache.org/r/43880/#comment187607> s/of/or/ Did you do a self review? ;) docs/monitoring.md (line 908) <https://reviews.apache.org/r/43880/#comment187606> s/of/or/ src/master/allocator/mesos/hierarchical.hpp (lines 290 - 294) <https://reviews.apache.org/r/43880/#comment187608> Let's perhaps include the word resources when naming these, and omit the comments (since we comment the metric insted of the function in the existing code). Let's do a s/resourceName/resource/ to be consistent with your last patch. How about `s/_total/_resources_total/` and `s/_allocated/_resources_allocated/` here and on the gauge variable names to be a bit more clear? It's also consistent with the master's approach to naming these gauges (note that this also fits well with naming the metric with a "resources/" prefix). src/master/allocator/mesos/hierarchical.cpp (line 1691) <https://reviews.apache.org/r/43880/#comment187622> s/resourceName/resource/ s/std::// src/master/allocator/mesos/hierarchical.cpp (line 1700) <https://reviews.apache.org/r/43880/#comment187618> s/.get()./->/ src/master/allocator/mesos/hierarchical.cpp (line 1708) <https://reviews.apache.org/r/43880/#comment187621> s/resourceName/resource/ s/std::// src/master/allocator/mesos/hierarchical.cpp (line 1713) <https://reviews.apache.org/r/43880/#comment187617> s/.get()./->/ src/master/allocator/mesos/hierarchical.cpp (line 1715) <https://reviews.apache.org/r/43880/#comment187624> two newlines here src/master/allocator/mesos/metrics.hpp (lines 58 - 62) <https://reviews.apache.org/r/43880/#comment187616> s/kind// How about `resources_total` and `resources_offered_or_allocated`? src/master/allocator/mesos/metrics.cpp (lines 50 - 53) <https://reviews.apache.org/r/43880/#comment187610> Let's also pull in dominic's TODO about dynamically adding gauges: ``` // Create resource gauges. // // TODO(bbannier) Add support for more than just scalar resources. // TODO(bbannier) Simplify this once MESOS-3214 is fixed. // TODO(dhamon): Set these up dynamically when adding a slave based on the // resources the slave exposes. ``` src/master/allocator/mesos/metrics.cpp (lines 54 - 55) <https://reviews.apache.org/r/43880/#comment187612> s/resourceKinds/resources/ (since you called the singular loop variable 'resource' already.. :)) src/master/allocator/mesos/metrics.cpp (line 55) <https://reviews.apache.org/r/43880/#comment187613> newline here? src/master/allocator/mesos/metrics.cpp (lines 56 - 61) <https://reviews.apache.org/r/43880/#comment187615> We don't need a map right now, we can just use a vector to be consistent with the master's metrics. We can switch to a map when we start dynamically adding gauges. src/master/allocator/mesos/metrics.cpp (lines 56 - 74) <https://reviews.apache.org/r/43880/#comment187620> How about the following structure, which is similar to the master's metrics and seems a bit cleaner? ``` foreach (const string& resource, resources) { Gauge total( "allocator/mesos/resources/" + resource + "/total", defer(allocator, &HierarchicalAllocatorProcess::_resources_total, resource)); Gauge offered_or_allocated( "allocator/mesos/resources/" + resource + "/offered_or_allocated", defer(allocator, &HierarchicalAllocatorProcess::_resources_offered_or_allocated, resource)); resources_total.push_back(total); resources_offered_or_allocated.push_back(offered_or_allocated); process::metrics::add(total); process::metrics::add(offered_or_allocated); } ``` src/master/allocator/mesos/metrics.cpp (line 106) <https://reviews.apache.org/r/43880/#comment187623> Whoops, I missed these earlier, no need for std:: qualifiers here and below. src/tests/hierarchical_allocator_tests.cpp (lines 2437 - 2438) <https://reviews.apache.org/r/43880/#comment187626> Try to wrap to reduce jaggedness: ``` // This test checks that total and allocator resources // are correctly reflected in the metrics endpoint. ``` src/tests/hierarchical_allocator_tests.cpp (lines 2437 - 2486) <https://reviews.apache.org/r/43880/#comment187628> Hm.. seems contains is more readable? Especially once the "resources/" prefix gets added because a lot of lines need to get wrapped: ``` // This test checks that total and allocator resources // are correctly reflected in the metrics endpoint. TEST_F(HierarchicalAllocatorTest, ResourceMetrics) { // Pausing the clock is not necessary, but ensures that the test // doesn't rely on the batch allocation in the allocator, which // would slow down the test. Clock::pause(); initialize(); SlaveInfo agent = createSlaveInfo("cpus:2;mem:1024;disk:0"); allocator->addSlave(agent.id(), agent, None(), agent.resources(), {}); Clock::settle(); JSON::Object expected; // No frameworks are registered yet, so nothing is allocated. expected.values = { {"allocator/mesos/resources/cpus/total", 2}, {"allocator/mesos/resources/mem/total", 1024}, {"allocator/mesos/resources/disk/total", 0}, {"allocator/mesos/resources/cpus/offered_or_allocated", 0}, {"allocator/mesos/resources/mem/offered_or_allocated", 0}, {"allocator/mesos/resources/disk/offered_or_allocated", 0}, }; JSON::Value metrics = Metrics(); EXPECT_TRUE(metrics.contains(expected)); FrameworkInfo framework = createFrameworkInfo("role1"); allocator->addFramework(framework.id(), framework, {}); Clock::settle(); // All of the resources should be offered. expected.values = { {"allocator/mesos/resources/cpus/total", 2}, {"allocator/mesos/resources/mem/total", 1024}, {"allocator/mesos/resources/disk/total", 0}, {"allocator/mesos/resources/cpus/offered_or_allocated", 2}, {"allocator/mesos/resources/mem/offered_or_allocated", 1024}, {"allocator/mesos/resources/disk/offered_or_allocated", 0}, }; metrics = Metrics(); EXPECT_TRUE(metrics.contains(expected)); allocator->removeSlave(agent.id()); Clock::settle(); // No frameworks are registered yet, so nothing is allocated. expected.values = { {"allocator/mesos/resources/cpus/total", 0}, {"allocator/mesos/resources/mem/total", 0}, {"allocator/mesos/resources/disk/total", 0}, {"allocator/mesos/resources/cpus/offered_or_allocated", 0}, {"allocator/mesos/resources/mem/offered_or_allocated", 0}, {"allocator/mesos/resources/disk/offered_or_allocated", 0}, }; metrics = Metrics(); EXPECT_TRUE(metrics.contains(expected)); } ``` - 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/43880/ > ----------------------------------------------------------- > > (Updated March 21, 2016, 11:08 a.m.) > > > Review request for mesos, Alexander Rukletsov and Ben Mahler. > > > Bugs: MESOS-4720 > https://issues.apache.org/jira/browse/MESOS-4720 > > > Repository: mesos > > > Description > ------- > > Added allocated metrics for total and allocated scalar resources. > > > Diffs > ----- > > docs/monitoring.md 827f7073204fcf8575ca980a5571c8be4f5e4110 > src/master/allocator/mesos/hierarchical.hpp > 0c622b8569bc79ae4e365a57e463ca5349356713 > 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/43880/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 > >
