You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mesos.apache.org by Jiang Yan Xu <ya...@jxu.me> on 2013/11/20 02:20:48 UTC
Review Request 15710: Fixed a bug in ZooKeeperMasterContenderDetectorTest
that caused the local timeout in Group not getting triggered.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/15710/
-----------------------------------------------------------
Review request for mesos and Ben Mahler.
Bugs: MESOS-823
https://issues.apache.org/jira/browse/MESOS-823
Repository: mesos-git
Description
-------
See summary.
Diffs
-----
src/tests/master_contender_detector_tests.cpp 5e4237454133edc155e74ffa04aec24ccd04c1b4
Diff: https://reviews.apache.org/r/15710/diff/
Testing
-------
make check 100 iterations
Thanks,
Jiang Yan Xu
Re: Review Request 15710: Fixed a bug in
ZooKeeperMasterContenderDetectorTest that caused the local timeout in Group
not getting triggered.
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/15710/#review29156
-----------------------------------------------------------
src/tests/master_contender_detector_tests.cpp
<https://reviews.apache.org/r/15710/#comment56297>
Looks good, we should consider creating a testing abstraction to make sure that our tests do not run forever:
DO_FOR (Seconds(10)) {
...
}
- Ben Mahler
On Nov. 20, 2013, 1:20 a.m., Jiang Yan Xu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/15710/
> -----------------------------------------------------------
>
> (Updated Nov. 20, 2013, 1:20 a.m.)
>
>
> Review request for mesos and Ben Mahler.
>
>
> Bugs: MESOS-823
> https://issues.apache.org/jira/browse/MESOS-823
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> src/tests/master_contender_detector_tests.cpp 5e4237454133edc155e74ffa04aec24ccd04c1b4
>
> Diff: https://reviews.apache.org/r/15710/diff/
>
>
> Testing
> -------
>
> make check 100 iterations
>
>
> Thanks,
>
> Jiang Yan Xu
>
>
Re: Review Request 15710: Fixed ZooKeeperMasterContenderDetectorTest to use
LOOP_FOR to ensure tests are bounded.
Posted by Jiang Yan Xu <ya...@jxu.me>.
> On Nov. 22, 2013, 2:58 a.m., Vinod Kone wrote:
> > src/tests/master_contender_detector_tests.cpp, line 433
> > <https://reviews.apache.org/r/15710/diff/2/?file=389605#file389605line433>
> >
> > I don't know if I like this abstraction.
> >
> > How about using the AWAIT_ macro here? If that doesn't work with paused clocks we should fix that instead.
>
> Ben Mahler wrote:
> I'm also in favor of this, we would have to enhance or add to the AWAIT_ macros for paused clocks to loop advancing the clock (possibly by a provided duration) until the provided future transitions from pending.
> Such a macro would indeed be better than LOOP_FOR and would be very useful for tests that have a clock advance loop in paused tests.
>
> However, I'm interested in tests like isolator_test.cpp, which cannot use an AWAIT_ style macro. It currently does exactly what LOOP_FOR is doing. It's possible that this is the only example so perhaps we will want to avoid adding this for a single test.
I think fixing AWAIT_ solves another type of problems but in this case (and in some in e.g. SlaveRecoveryTest) I can't use AWAIT_ either.
You wait because a delay is involved and you want to advance the lock to avoid really wait for that long.
You have to advance repeatedly (because you may have advanced the clock before delay is called).
Otherwise you have to first wait for delay to get called which FUTURE_DISPATCH cannot accomplish.
- Jiang Yan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/15710/#review29275
-----------------------------------------------------------
On Nov. 21, 2013, 11:18 p.m., Jiang Yan Xu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/15710/
> -----------------------------------------------------------
>
> (Updated Nov. 21, 2013, 11:18 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, Ben Mahler, and Vinod Kone.
>
>
> Bugs: MESOS-823
> https://issues.apache.org/jira/browse/MESOS-823
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> src/tests/master_contender_detector_tests.cpp 5e4237454133edc155e74ffa04aec24ccd04c1b4
> src/tests/zookeeper_tests.cpp 0059438a26bfd03ee6da15029ca7a67674ea637e
>
> Diff: https://reviews.apache.org/r/15710/diff/
>
>
> Testing
> -------
>
> make check Linux (100 iterations) and OSX.
>
>
> Thanks,
>
> Jiang Yan Xu
>
>
Re: Review Request 15710: Fixed ZooKeeperMasterContenderDetectorTest to use
LOOP_FOR to ensure tests are bounded.
Posted by Ben Mahler <be...@gmail.com>.
> On Nov. 22, 2013, 2:58 a.m., Vinod Kone wrote:
> > src/tests/master_contender_detector_tests.cpp, line 433
> > <https://reviews.apache.org/r/15710/diff/2/?file=389605#file389605line433>
> >
> > I don't know if I like this abstraction.
> >
> > How about using the AWAIT_ macro here? If that doesn't work with paused clocks we should fix that instead.
I'm also in favor of this, we would have to enhance or add to the AWAIT_ macros for paused clocks to loop advancing the clock (possibly by a provided duration) until the provided future transitions from pending.
Such a macro would indeed be better than LOOP_FOR and would be very useful for tests that have a clock advance loop in paused tests.
However, I'm interested in tests like isolator_test.cpp, which cannot use an AWAIT_ style macro. It currently does exactly what LOOP_FOR is doing. It's possible that this is the only example so perhaps we will want to avoid adding this for a single test.
- Ben
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/15710/#review29275
-----------------------------------------------------------
On Nov. 21, 2013, 11:18 p.m., Jiang Yan Xu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/15710/
> -----------------------------------------------------------
>
> (Updated Nov. 21, 2013, 11:18 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, Ben Mahler, and Vinod Kone.
>
>
> Bugs: MESOS-823
> https://issues.apache.org/jira/browse/MESOS-823
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> src/tests/master_contender_detector_tests.cpp 5e4237454133edc155e74ffa04aec24ccd04c1b4
> src/tests/zookeeper_tests.cpp 0059438a26bfd03ee6da15029ca7a67674ea637e
>
> Diff: https://reviews.apache.org/r/15710/diff/
>
>
> Testing
> -------
>
> make check Linux (100 iterations) and OSX.
>
>
> Thanks,
>
> Jiang Yan Xu
>
>
Re: Review Request 15710: Fixed ZooKeeperMasterContenderDetectorTest to use
LOOP_FOR to ensure tests are bounded.
Posted by Ben Mahler <be...@gmail.com>.
> On Nov. 22, 2013, 2:58 a.m., Vinod Kone wrote:
> > src/tests/master_contender_detector_tests.cpp, line 433
> > <https://reviews.apache.org/r/15710/diff/2/?file=389605#file389605line433>
> >
> > I don't know if I like this abstraction.
> >
> > How about using the AWAIT_ macro here? If that doesn't work with paused clocks we should fix that instead.
>
> Ben Mahler wrote:
> I'm also in favor of this, we would have to enhance or add to the AWAIT_ macros for paused clocks to loop advancing the clock (possibly by a provided duration) until the provided future transitions from pending.
> Such a macro would indeed be better than LOOP_FOR and would be very useful for tests that have a clock advance loop in paused tests.
>
> However, I'm interested in tests like isolator_test.cpp, which cannot use an AWAIT_ style macro. It currently does exactly what LOOP_FOR is doing. It's possible that this is the only example so perhaps we will want to avoid adding this for a single test.
>
> Jiang Yan Xu wrote:
> I think fixing AWAIT_ solves another type of problems but in this case (and in some in e.g. SlaveRecoveryTest) I can't use AWAIT_ either.
>
> You wait because a delay is involved and you want to advance the lock to avoid really wait for that long.
> You have to advance repeatedly (because you may have advanced the clock before delay is called).
>
> Otherwise you have to first wait for delay to get called which FUTURE_DISPATCH cannot accomplish.
Let's imagine you have something that handles a paused clock, like:
AWAIT_READY'(future, interval, limit)
{
Duration total;
if (Clock::paused()) {
while (future.isPending() && total < limit) {
Clock::advance(interval);
Clock::settle();
total += interval;
}
} else {
...
}
}
Would this not help in this test?
- Ben
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/15710/#review29275
-----------------------------------------------------------
On Nov. 21, 2013, 11:18 p.m., Jiang Yan Xu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/15710/
> -----------------------------------------------------------
>
> (Updated Nov. 21, 2013, 11:18 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, Ben Mahler, and Vinod Kone.
>
>
> Bugs: MESOS-823
> https://issues.apache.org/jira/browse/MESOS-823
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> src/tests/master_contender_detector_tests.cpp 5e4237454133edc155e74ffa04aec24ccd04c1b4
> src/tests/zookeeper_tests.cpp 0059438a26bfd03ee6da15029ca7a67674ea637e
>
> Diff: https://reviews.apache.org/r/15710/diff/
>
>
> Testing
> -------
>
> make check Linux (100 iterations) and OSX.
>
>
> Thanks,
>
> Jiang Yan Xu
>
>
Re: Review Request 15710: Fixed ZooKeeperMasterContenderDetectorTest to use
LOOP_FOR to ensure tests are bounded.
Posted by Jiang Yan Xu <ya...@jxu.me>.
> On Nov. 21, 2013, 6:58 p.m., Vinod Kone wrote:
> > src/tests/master_contender_detector_tests.cpp, line 433
> > <https://reviews.apache.org/r/15710/diff/2/?file=389605#file389605line433>
> >
> > I don't know if I like this abstraction.
> >
> > How about using the AWAIT_ macro here? If that doesn't work with paused clocks we should fix that instead.
>
> Ben Mahler wrote:
> I'm also in favor of this, we would have to enhance or add to the AWAIT_ macros for paused clocks to loop advancing the clock (possibly by a provided duration) until the provided future transitions from pending.
> Such a macro would indeed be better than LOOP_FOR and would be very useful for tests that have a clock advance loop in paused tests.
>
> However, I'm interested in tests like isolator_test.cpp, which cannot use an AWAIT_ style macro. It currently does exactly what LOOP_FOR is doing. It's possible that this is the only example so perhaps we will want to avoid adding this for a single test.
>
> Jiang Yan Xu wrote:
> I think fixing AWAIT_ solves another type of problems but in this case (and in some in e.g. SlaveRecoveryTest) I can't use AWAIT_ either.
>
> You wait because a delay is involved and you want to advance the lock to avoid really wait for that long.
> You have to advance repeatedly (because you may have advanced the clock before delay is called).
>
> Otherwise you have to first wait for delay to get called which FUTURE_DISPATCH cannot accomplish.
>
> Ben Mahler wrote:
> Let's imagine you have something that handles a paused clock, like:
>
> AWAIT_READY'(future, interval, limit)
>
> {
> Duration total;
> if (Clock::paused()) {
> while (future.isPending() && total < limit) {
> Clock::advance(interval);
> Clock::settle();
> total += interval;
> }
> } else {
> ...
> }
> }
>
> Would this not help in this test?
Ultimately the goal is to prevent tests from running for more than the specified duration of REAL time.
The above approach does not read the system clock so it can still spin too fast and expire before some slow (network/io) event we are hoping to occur for this Future to be satisfied.
We can insert a os::sleep(interval) for every interval that we advance but this still doesn't feel sufficient: we may be advancing the clock too slowly and defeating the purpose of the clock manipulation. We do it to speed up tests.
Neither do I think we should "fix" future.await(duration) deep inside its call-chain.
The fix for AWAIT_READY is that it should look at the system time. I submitted a review at: https://reviews.apache.org/r/17805/
I found it very implicit and deprives the control over the clock from the developers who write the tests to hide Clock::advance() here.
So I still prefer an abstraction like LOOP_FOR to allow developers who need to advance the clock repeatedly in a loop to have an easy way to express an upper bound in terms of time.
If it is used in very few places and thus unnecessary, it's another issue :)
Thoughts?
- Jiang Yan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/15710/#review29275
-----------------------------------------------------------
On Nov. 21, 2013, 3:18 p.m., Jiang Yan Xu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/15710/
> -----------------------------------------------------------
>
> (Updated Nov. 21, 2013, 3:18 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, Ben Mahler, and Vinod Kone.
>
>
> Bugs: MESOS-823
> https://issues.apache.org/jira/browse/MESOS-823
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> src/tests/master_contender_detector_tests.cpp 5e4237454133edc155e74ffa04aec24ccd04c1b4
> src/tests/zookeeper_tests.cpp 0059438a26bfd03ee6da15029ca7a67674ea637e
>
> Diff: https://reviews.apache.org/r/15710/diff/
>
>
> Testing
> -------
>
> make check Linux (100 iterations) and OSX.
>
>
> Thanks,
>
> Jiang Yan Xu
>
>
Re: Review Request 15710: Fixed ZooKeeperMasterContenderDetectorTest to use
LOOP_FOR to ensure tests are bounded.
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/15710/#review29275
-----------------------------------------------------------
src/tests/master_contender_detector_tests.cpp
<https://reviews.apache.org/r/15710/#comment56429>
I don't know if I like this abstraction.
How about using the AWAIT_ macro here? If that doesn't work with paused clocks we should fix that instead.
- Vinod Kone
On Nov. 21, 2013, 11:18 p.m., Jiang Yan Xu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/15710/
> -----------------------------------------------------------
>
> (Updated Nov. 21, 2013, 11:18 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, Ben Mahler, and Vinod Kone.
>
>
> Bugs: MESOS-823
> https://issues.apache.org/jira/browse/MESOS-823
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> src/tests/master_contender_detector_tests.cpp 5e4237454133edc155e74ffa04aec24ccd04c1b4
> src/tests/zookeeper_tests.cpp 0059438a26bfd03ee6da15029ca7a67674ea637e
>
> Diff: https://reviews.apache.org/r/15710/diff/
>
>
> Testing
> -------
>
> make check Linux (100 iterations) and OSX.
>
>
> Thanks,
>
> Jiang Yan Xu
>
>
Re: Review Request 15710: Fixed ZooKeeperMasterContenderDetectorTest to use
LOOP_FOR to ensure tests are bounded.
Posted by Jiang Yan Xu <ya...@jxu.me>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/15710/
-----------------------------------------------------------
(Updated Nov. 21, 2013, 11:18 p.m.)
Review request for mesos, Benjamin Hindman, Ben Mahler, and Vinod Kone.
Changes
-------
Now use LOOP_FOR for tests with loops.
Summary (updated)
-----------------
Fixed ZooKeeperMasterContenderDetectorTest to use LOOP_FOR to ensure tests are bounded.
Bugs: MESOS-823
https://issues.apache.org/jira/browse/MESOS-823
Repository: mesos-git
Description
-------
See summary.
Diffs (updated)
-----
src/tests/master_contender_detector_tests.cpp 5e4237454133edc155e74ffa04aec24ccd04c1b4
src/tests/zookeeper_tests.cpp 0059438a26bfd03ee6da15029ca7a67674ea637e
Diff: https://reviews.apache.org/r/15710/diff/
Testing (updated)
-------
make check Linux (100 iterations) and OSX.
Thanks,
Jiang Yan Xu