> On June 4, 2019, 8:42 p.m., Joseph Wu wrote:
> > I think it is a little unfortunate that we end up with a conditional
> > `select()` and an extra chaining step in this endpoint. But since we
> > aren't saving the vector of keys/futures/statistics anywhere, the chaining
> > is mostly unavoidable.
> >
> > Since this adds some significant logic, I would like there to be at least
> > an addition to the tests. We can't read the resulting log line, but we can
> > at least exercise the code. For example, copying this chunk of existing
> > test code and increasing the in-test timeout to 10+ seconds:
> >
> > ```
> > TEST_F(MetricsTest, THREADSAFE_SnapshotTimeout)
> > {
> > ...
> >
> > // Get the snapshot.
> > Future<Response> response = http::get(upid, "snapshot", "timeout=2secs");
> > // <----------------- Increase this timeout.
> >
> > // Make sure the request is pending before the timeout is exceeded.
> > //
> > // TODO(neilc): Replace the `sleep` here with a less flaky
> > // synchronization method.
> > os::sleep(Milliseconds(10));
> > Clock::settle();
> >
> > ASSERT_TRUE(response.isPending());
> >
> > // Advance the clock to trigger the timeout.
> > Clock::advance(Seconds(2)); // <----------------------------- And this
> > timeout.
> >
> > AWAIT_EXPECT_RESPONSE_STATUS_EQ(OK().status, response);
> >
> > // Parse the response.
> > Try<JSON::Object> responseJSON =
> > JSON::parse<JSON::Object>(response->body);
> > ASSERT_SOME(responseJSON);
> >
> > // We can't use simple JSON equality testing here as initializing
> > // libprocess adds metrics to the system. We want to only check if
> > // the metrics from this test are correctly handled.
> > map<string, JSON::Value> values = responseJSON->values;
> >
> > EXPECT_EQ(1u, values.count("test/counter"));
> > EXPECT_DOUBLE_EQ(0.0,
> > values["test/counter"].as<JSON::Number>().as<double>());
> >
> > EXPECT_EQ(1u, values.count("test/gauge"));
> > EXPECT_DOUBLE_EQ(42.0,
> > values["test/gauge"].as<JSON::Number>().as<double>());
> >
> > EXPECT_EQ(0u, values.count("test/gauge_fail"));
> > EXPECT_EQ(0u, values.count("test/gauge_timeout"));
> > ```
Good call, I updated the test.
- Greg
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70783/#review215689
-----------------------------------------------------------
On June 5, 2019, 3:39 p.m., Greg Mann wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70783/
> -----------------------------------------------------------
>
> (Updated June 5, 2019, 3:39 p.m.)
>
>
> Review request for mesos, Benno Evers, Benjamin Mahler, Joseph Wu, and Vinod
> Kone.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added debug logging for metrics which are slow to become ready.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/metrics/metrics.hpp
> 75711edbaf46797e5eb54ba720ea11cf3de81522
> 3rdparty/libprocess/src/metrics/metrics.cpp
> 623d44adbe838f995ddbe89ee26f5bcc9c600be5
> 3rdparty/libprocess/src/tests/metrics_tests.cpp
> 881275693e67f3c9fb670c7e70cb5014090ed7a5
>
>
> Diff: https://reviews.apache.org/r/70783/diff/3/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Greg Mann
>
>
