----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/53840/#review175817 -----------------------------------------------------------
Ship it! I'll commit with these adjustments. docs/monitoring.md Lines 1053 (patched) <https://reviews.apache.org/r/53840/#comment249208> s/algorithm/batch/ docs/monitoring.md Lines 1053 (patched) <https://reviews.apache.org/r/53840/#comment249210> s/algorithm/batch/ docs/monitoring.md Lines 1058 (patched) <https://reviews.apache.org/r/53840/#comment249209> Having all these lines for each timer feels like too much repetition but alas, it's the pattern right now and let's just maintain consistency and clean up later. src/tests/hierarchical_allocator_tests.cpp Lines 3578-3581 (original), 3578-3581 (patched) <https://reviews.apache.org/r/53840/#comment249149> I kept the comment but do see the need to move the assignment. src/tests/hierarchical_allocator_tests.cpp Lines 3673 (patched) <https://reviews.apache.org/r/53840/#comment249205> I simplified the test a bit. Until an abstraction is provided, tests tend to be copied so let's keep things as simple as possible. ``` TEST_F_TEMP_DISABLED_ON_WINDOWS( HierarchicalAllocatorTest, AllocationRunLatencyMetrics) { Clock::pause(); initialize(); // These time series statistics will be generated // once at least 2 allocation runs occur. auto statistics = { "allocator/mesos/allocation_run_latency_ms/count", "allocator/mesos/allocation_run_latency_ms/min", "allocator/mesos/allocation_run_latency_ms/max", "allocator/mesos/allocation_run_latency_ms/p50", "allocator/mesos/allocation_run_latency_ms/p95", "allocator/mesos/allocation_run_latency_ms/p99", "allocator/mesos/allocation_run_latency_ms/p999", "allocator/mesos/allocation_run_latency_ms/p9999", }; JSON::Object metrics = Metrics(); map<string, JSON::Value> values = metrics.values; EXPECT_EQ(0u, values.count("allocator/mesos/allocation_run_latency_ms")); // No allocation latency statistics should appear. foreach (const string& statistic, statistics) { EXPECT_EQ(0u, values.count(statistic)) << "Expected " << statistic << " to be absent"; } // Allow the allocation timer to measure time. Clock::resume(); // Trigger at least two calls to allocate to generate the window statistics. SlaveInfo agent = createSlaveInfo("cpus:2;mem:1024;disk:0"); allocator->addSlave( agent.id(), agent, AGENT_CAPABILITIES(), None(), agent.resources(), {}); // Wait for the allocation triggered by `addSlave()` to complete. Clock::pause(); Clock::settle(); Clock::resume(); FrameworkInfo framework = createFrameworkInfo({"role1"}); allocator->addFramework(framework.id(), framework, {}, true); // Wait for the allocation triggered by `addFramework()` to complete. AWAIT_READY(allocations.get()); metrics = Metrics(); values = metrics.values; // A non-zero measurement should be present. EXPECT_EQ(1u, values.count("allocator/mesos/allocation_run_latency_ms")); JSON::Value value = metrics.values["allocator/mesos/allocation_run_latency_ms"]; ASSERT_TRUE(value.is<JSON::Number>()) << value.which(); JSON::Number timing = value.as<JSON::Number>(); ASSERT_EQ(JSON::Number::FLOATING, timing.type); EXPECT_GE(timing.as<double>(), 0.0); // The statistics should be generated. foreach (const string& statistic, statistics) { EXPECT_EQ(1u, values.count(statistic)) << "Expected " << statistic << " to be present"; } } ``` - Jiang Yan Xu On May 23, 2017, 9:30 a.m., Anindya Sinha wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/53840/ > ----------------------------------------------------------- > > (Updated May 23, 2017, 9:30 a.m.) > > > Review request for mesos, James Peach and Jiang Yan Xu. > > > Bugs: MESOS-6579 > https://issues.apache.org/jira/browse/MESOS-6579 > > > Repository: mesos > > > Description > ------- > > Added a metric 'allocator/mesos/allocation_run_interval_ms' which > tracks the latency between scheduling an allocation to an actual > allocation run. > > > Diffs > ----- > > docs/monitoring.md a027f4905a0e6e41ff4e1348d34fd7aa5f1cbe61 > src/master/allocator/mesos/hierarchical.cpp > b75ed9a20a9a42f958cebbacd91e5e15b0d21394 > src/master/allocator/mesos/metrics.hpp > ce486eb079dc44bf0bbfad8668426c7ae87c97b3 > src/master/allocator/mesos/metrics.cpp > ec987fe0ce8605bf9e7c0ccbe3ac384deaf53be1 > src/tests/hierarchical_allocator_tests.cpp > 6dee2296d5a14185dbf7eee17968b20148839bfd > > > Diff: https://reviews.apache.org/r/53840/diff/7/ > > > Testing > ------- > > All tests passed. > > > Thanks, > > Anindya Sinha > >
