You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Neil Conway <ne...@gmail.com> on 2016/01/10 21:33:42 UTC
Review Request 42121: Replaced `Clock::settle()` with `os::sleep` in
test case.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/42121/
-----------------------------------------------------------
Review request for mesos, Ben Mahler and Joris Van Remoortere.
Repository: mesos
Description
-------
Using `Clock::settle()` does not provide the synchronization that
"MetricsTest.SnapshotTimeout" requires. Of course, using `os::sleep()` is not
right either, but at least it is more honest than using `Clock::settle()`.
Diffs
-----
3rdparty/libprocess/src/tests/metrics_tests.cpp b84dc8d858f58bc9f52b218b7153510417cf34c2
Diff: https://reviews.apache.org/r/42121/diff/
Testing
-------
./3rdparty/libprocess/libprocess-tests --gtest_filter="MetricsTest.SnapshotTimeout" --gtest_repeat=2000 --gtest_break_on_failure
Thanks,
Neil Conway
Re: Review Request 42121: Replaced `Clock::settle()` with `os::sleep`
in test case.
Posted by Neil Conway <ne...@gmail.com>.
> On Jan. 11, 2016, 8:09 p.m., Joseph Wu wrote:
> > 3rdparty/libprocess/src/tests/metrics_tests.cpp, lines 280-283
> > <https://reviews.apache.org/r/42121/diff/1/?file=1190948#file1190948line280>
> >
> > Just some thoughts:
> >
> > The downstream event you're waiting for is a call to `Clock::timer` with a duration of 2 seconds. You can presumably access the private clock variables `timers` and `timers_mutex` by declaring them in the test. (We do with for `openssl::reinitialize`.)
> >
> > If you do this, you'll probably need to loop and check for a specific time to be added (`Timeout::in(duration)`). To make the check deterministic, the clock should be paused (so that `Clock::now` stays constant), which it already is in the test.
Yeah, that would work but it seems pretty kludgy and would be a layering violation.
It would be nicer if we could mock the MetricsProcess and then use the existing `EXPECT_CALL` machinery to ensure that sufficient progress has been made, but that would require a bunch of refactoring.
- Neil
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/42121/#review113827
-----------------------------------------------------------
On Jan. 10, 2016, 8:33 p.m., Neil Conway wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42121/
> -----------------------------------------------------------
>
> (Updated Jan. 10, 2016, 8:33 p.m.)
>
>
> Review request for mesos, Ben Mahler and Joris Van Remoortere.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Using `Clock::settle()` does not provide the synchronization that
> "MetricsTest.SnapshotTimeout" requires. Of course, using `os::sleep()` is not
> right either, but at least it is more honest than using `Clock::settle()`.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/src/tests/metrics_tests.cpp b84dc8d858f58bc9f52b218b7153510417cf34c2
>
> Diff: https://reviews.apache.org/r/42121/diff/
>
>
> Testing
> -------
>
> ./3rdparty/libprocess/libprocess-tests --gtest_filter="MetricsTest.SnapshotTimeout" --gtest_repeat=2000 --gtest_break_on_failure
>
>
> Thanks,
>
> Neil Conway
>
>
Re: Review Request 42121: Replaced `Clock::settle()` with `os::sleep`
in test case.
Posted by Joseph Wu <jo...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/42121/#review113827
-----------------------------------------------------------
3rdparty/libprocess/src/tests/metrics_tests.cpp (lines 280 - 283)
<https://reviews.apache.org/r/42121/#comment174618>
Just some thoughts:
The downstream event you're waiting for is a call to `Clock::timer` with a duration of 2 seconds. You can presumably access the private clock variables `timers` and `timers_mutex` by declaring them in the test. (We do with for `openssl::reinitialize`.)
If you do this, you'll probably need to loop and check for a specific time to be added (`Timeout::in(duration)`). To make the check deterministic, the clock should be paused (so that `Clock::now` stays constant), which it already is in the test.
- Joseph Wu
On Jan. 10, 2016, 12:33 p.m., Neil Conway wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42121/
> -----------------------------------------------------------
>
> (Updated Jan. 10, 2016, 12:33 p.m.)
>
>
> Review request for mesos, Ben Mahler and Joris Van Remoortere.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Using `Clock::settle()` does not provide the synchronization that
> "MetricsTest.SnapshotTimeout" requires. Of course, using `os::sleep()` is not
> right either, but at least it is more honest than using `Clock::settle()`.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/src/tests/metrics_tests.cpp b84dc8d858f58bc9f52b218b7153510417cf34c2
>
> Diff: https://reviews.apache.org/r/42121/diff/
>
>
> Testing
> -------
>
> ./3rdparty/libprocess/libprocess-tests --gtest_filter="MetricsTest.SnapshotTimeout" --gtest_repeat=2000 --gtest_break_on_failure
>
>
> Thanks,
>
> Neil Conway
>
>
Re: Review Request 42121: Replaced `Clock::settle()` with `os::sleep`
in test case.
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/42121/#review113733
-----------------------------------------------------------
Patch looks great!
Reviews applied: [42121]
Passed command: export OS=ubuntu:14.04;export CONFIGURATION="--verbose";export COMPILER=gcc; ./support/docker_build.sh
- Mesos ReviewBot
On Jan. 10, 2016, 8:33 p.m., Neil Conway wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42121/
> -----------------------------------------------------------
>
> (Updated Jan. 10, 2016, 8:33 p.m.)
>
>
> Review request for mesos, Ben Mahler and Joris Van Remoortere.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Using `Clock::settle()` does not provide the synchronization that
> "MetricsTest.SnapshotTimeout" requires. Of course, using `os::sleep()` is not
> right either, but at least it is more honest than using `Clock::settle()`.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/src/tests/metrics_tests.cpp b84dc8d858f58bc9f52b218b7153510417cf34c2
>
> Diff: https://reviews.apache.org/r/42121/diff/
>
>
> Testing
> -------
>
> ./3rdparty/libprocess/libprocess-tests --gtest_filter="MetricsTest.SnapshotTimeout" --gtest_repeat=2000 --gtest_break_on_failure
>
>
> Thanks,
>
> Neil Conway
>
>