> On March 18, 2016, 11:24 p.m., Ben Mahler wrote:
> > src/tests/slave_tests.cpp, line 3264
> > <https://reviews.apache.org/r/44994/diff/2/?file=1306236#file1306236line3264>
> >
> >     You don't need a settle here, AWAIT_READY will settle if the clock is 
> > paused.
> 
> Alexander Rukletsov wrote:
>     I feel that this is not obvious, that settle will happen in `AWAIT_READY` 
> if clock is paused. Moreover, when we don't await, we have to explicitly 
> settle, which—I think—makes it harder for folks to follow the test and reason 
> when settle is necessary, because sometimes a necessary settle is implicit 
> (inside await). Hence I tend to put explicit settles when I feel this makes 
> the code more obvious. Do you have a strong preference?

Think about it this way, if you put a `settle()` before the `AWAIT_READY`, then 
the implication is that you don't need `AWAIT_READY` at all, you could just do 
`ASSERT_TRUE(f.isReady())` because you've done the necessary settling 
explicitly. This is why it seems strange to have a `settle` immediately preceed 
an `AWAIT_READY`, its the job of `AWAIT_READY` to wait until the future is 
ready (which means doing the right thing when the clock is paused, otherwise we 
would have to write a lot of `settle` calls).


> On March 18, 2016, 11:24 p.m., Ben Mahler wrote:
> > src/tests/slave_tests.cpp, line 3357
> > <https://reviews.apache.org/r/44994/diff/2/?file=1306236#file1306236line3357>
> >
> >     Why do you resume?
> 
> Alexander Rukletsov wrote:
>     I think we *should* resume the clock before exiting the test. I've heard 
> plans to even add a check in our test harness that verified the clock is not 
> paused.

This is similar to having to do an explicit Shutdown() at the end of the test. 
If the test fails, the Shutdown() (or Clock::resume()) better be called! 
Looking around it seems like we have this in a bunch of places, which is 
unfortunate because if an assertion fails the clock is left paused. I thought 
we ensured it was resumed but I can't find that code, so I'm ok with leaving 
this in.


- Ben


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/44994/#review124308
-----------------------------------------------------------


On March 22, 2016, 5:07 p.m., Alexander Rukletsov wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44994/
> -----------------------------------------------------------
> 
> (Updated March 22, 2016, 5:07 p.m.)
> 
> 
> Review request for mesos and Ben Mahler.
> 
> 
> Bugs: MESOS-4949
>     https://issues.apache.org/jira/browse/MESOS-4949
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> See summary.
> 
> 
> Diffs
> -----
> 
>   src/slave/slave.hpp 7520cc356e2b1b7f5fff15f33071a46a7b05e762 
>   src/tests/slave_tests.cpp ea1d776077bf638885db8421194aa4427c772169 
> 
> Diff: https://reviews.apache.org/r/44994/diff/
> 
> 
> Testing
> -------
> 
> On Mac OS 10.10.4:
> `make check`
> `GTEST_FILTER="*ExecutorShutdownGracePeriod*" ./bin/mesos-tests.sh 
> --gtest_repeat=100 --gtest_break_on_failure`
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Reply via email to