> 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 > >
