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 2017/05/25 18:44:35 UTC
Review Request 59578: Made
MasterTest.MaxCompletedTasksPerFrameworkFlag less fragile.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59578/
-----------------------------------------------------------
Review request for mesos and Kevin Klues.
Repository: mesos
Description
-------
Rather than depending on batch allocations to eventually occur, pause
the clock and explicitly advance it when we want to trigger a batch
allocation. This improves both test robustness and speed.
Diffs
-----
src/tests/master_tests.cpp 1dfe5fd7c33c3c479175aa522fef1956f11f265c
Diff: https://reviews.apache.org/r/59578/diff/1/
Testing
-------
`make check`
Thanks,
Neil Conway
Re: Review Request 59578: Made
MasterTest.MaxCompletedTasksPerFrameworkFlag less fragile.
Posted by Neil Conway <ne...@gmail.com>.
> On May 26, 2017, 12:03 a.m., Kevin Klues wrote:
> > src/tests/master_tests.cpp
> > Lines 5635-5636 (patched)
> > <https://reviews.apache.org/r/59578/diff/1/?file=1732877#file1732877line5635>
> >
> > Is this something standard we've been doing recently to fix flaky tests? I'm not familiar with this pattern. of pausing the clocl and then advancing it manually.
We do this fairly often in the tests (`ag "Clock::pause" src/tests | wc -l` => 366). In my opinion, the advantages are:
* Tests that rely on the clock making progress are more likely to be flaky, because there is no synchronization between how fast the test executes and when clock-based timers go off.
* A test that is written to assume that clock advances at a certain point has an _implicit_ timing dependency. Pausing the clock and advancing the clock makes this dependency explicit.
* The test usually runs faster; rather than waiting for the system clock (which might be quite a while), we can immediately advance the clock the appropriate amount.
Personally, I think we should pause the clock in all tests by default (MESOS-4101).
- Neil
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59578/#review176154
-----------------------------------------------------------
On May 25, 2017, 6:44 p.m., Neil Conway wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59578/
> -----------------------------------------------------------
>
> (Updated May 25, 2017, 6:44 p.m.)
>
>
> Review request for mesos and Kevin Klues.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Rather than depending on batch allocations to eventually occur, pause
> the clock and explicitly advance it when we want to trigger a batch
> allocation. This improves both test robustness and speed.
>
>
> Diffs
> -----
>
> src/tests/master_tests.cpp 1dfe5fd7c33c3c479175aa522fef1956f11f265c
>
>
> Diff: https://reviews.apache.org/r/59578/diff/1/
>
>
> Testing
> -------
>
> `make check`
>
>
> Thanks,
>
> Neil Conway
>
>
Re: Review Request 59578: Made
MasterTest.MaxCompletedTasksPerFrameworkFlag less fragile.
Posted by Kevin Klues <kl...@gmail.com>.
> On May 26, 2017, 12:03 a.m., Kevin Klues wrote:
> > src/tests/master_tests.cpp
> > Lines 5635-5636 (patched)
> > <https://reviews.apache.org/r/59578/diff/1/?file=1732877#file1732877line5635>
> >
> > Is this something standard we've been doing recently to fix flaky tests? I'm not familiar with this pattern. of pausing the clocl and then advancing it manually.
>
> Neil Conway wrote:
> We do this fairly often in the tests (`ag "Clock::pause" src/tests | wc -l` => 366). In my opinion, the advantages are:
>
> * Tests that rely on the clock making progress are more likely to be flaky, because there is no synchronization between how fast the test executes and when clock-based timers go off.
> * A test that is written to assume that clock advances at a certain point has an _implicit_ timing dependency. Pausing the clock and advancing the clock makes this dependency explicit.
> * The test usually runs faster; rather than waiting for the system clock (which might be quite a while), we can immediately advance the clock the appropriate amount.
>
> Personally, I think we should pause the clock in all tests by default (MESOS-4101).
Makes sense.
- Kevin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59578/#review176154
-----------------------------------------------------------
On May 25, 2017, 6:44 p.m., Neil Conway wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59578/
> -----------------------------------------------------------
>
> (Updated May 25, 2017, 6:44 p.m.)
>
>
> Review request for mesos and Kevin Klues.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Rather than depending on batch allocations to eventually occur, pause
> the clock and explicitly advance it when we want to trigger a batch
> allocation. This improves both test robustness and speed.
>
>
> Diffs
> -----
>
> src/tests/master_tests.cpp 1dfe5fd7c33c3c479175aa522fef1956f11f265c
>
>
> Diff: https://reviews.apache.org/r/59578/diff/1/
>
>
> Testing
> -------
>
> `make check`
>
>
> Thanks,
>
> Neil Conway
>
>
Re: Review Request 59578: Made
MasterTest.MaxCompletedTasksPerFrameworkFlag less fragile.
Posted by Kevin Klues <kl...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59578/#review176154
-----------------------------------------------------------
src/tests/master_tests.cpp
Lines 5635-5636 (patched)
<https://reviews.apache.org/r/59578/#comment249510>
Is this something standard we've been doing recently to fix flaky tests? I'm not familiar with this pattern. of pausing the clocl and then advancing it manually.
- Kevin Klues
On May 25, 2017, 6:44 p.m., Neil Conway wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59578/
> -----------------------------------------------------------
>
> (Updated May 25, 2017, 6:44 p.m.)
>
>
> Review request for mesos and Kevin Klues.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Rather than depending on batch allocations to eventually occur, pause
> the clock and explicitly advance it when we want to trigger a batch
> allocation. This improves both test robustness and speed.
>
>
> Diffs
> -----
>
> src/tests/master_tests.cpp 1dfe5fd7c33c3c479175aa522fef1956f11f265c
>
>
> Diff: https://reviews.apache.org/r/59578/diff/1/
>
>
> Testing
> -------
>
> `make check`
>
>
> Thanks,
>
> Neil Conway
>
>
Re: Review Request 59578: Made
MasterTest.MaxCompletedTasksPerFrameworkFlag less fragile.
Posted by Kevin Klues <kl...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59578/#review176242
-----------------------------------------------------------
Ship it!
Ship It!
- Kevin Klues
On May 25, 2017, 6:44 p.m., Neil Conway wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59578/
> -----------------------------------------------------------
>
> (Updated May 25, 2017, 6:44 p.m.)
>
>
> Review request for mesos and Kevin Klues.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Rather than depending on batch allocations to eventually occur, pause
> the clock and explicitly advance it when we want to trigger a batch
> allocation. This improves both test robustness and speed.
>
>
> Diffs
> -----
>
> src/tests/master_tests.cpp 1dfe5fd7c33c3c479175aa522fef1956f11f265c
>
>
> Diff: https://reviews.apache.org/r/59578/diff/1/
>
>
> Testing
> -------
>
> `make check`
>
>
> Thanks,
>
> Neil Conway
>
>
Re: Review Request 59578: Made
MasterTest.MaxCompletedTasksPerFrameworkFlag less fragile.
Posted by Mesos Reviewbot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59578/#review176179
-----------------------------------------------------------
Patch looks great!
Reviews applied: [59578]
Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' CONFIGURATION='--verbose' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker-build.sh
- Mesos Reviewbot
On May 25, 2017, 6:44 p.m., Neil Conway wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59578/
> -----------------------------------------------------------
>
> (Updated May 25, 2017, 6:44 p.m.)
>
>
> Review request for mesos and Kevin Klues.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Rather than depending on batch allocations to eventually occur, pause
> the clock and explicitly advance it when we want to trigger a batch
> allocation. This improves both test robustness and speed.
>
>
> Diffs
> -----
>
> src/tests/master_tests.cpp 1dfe5fd7c33c3c479175aa522fef1956f11f265c
>
>
> Diff: https://reviews.apache.org/r/59578/diff/1/
>
>
> Testing
> -------
>
> `make check`
>
>
> Thanks,
>
> Neil Conway
>
>
Re: Review Request 59578: Made
MasterTest.MaxCompletedTasksPerFrameworkFlag less fragile.
Posted by Mesos Reviewbot Windows <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59578/#review176326
-----------------------------------------------------------
Patch looks great!
Reviews applied: [59578]
Passed command: support\windows-build.bat
- Mesos Reviewbot Windows
On May 25, 2017, 6:44 p.m., Neil Conway wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59578/
> -----------------------------------------------------------
>
> (Updated May 25, 2017, 6:44 p.m.)
>
>
> Review request for mesos and Kevin Klues.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Rather than depending on batch allocations to eventually occur, pause
> the clock and explicitly advance it when we want to trigger a batch
> allocation. This improves both test robustness and speed.
>
>
> Diffs
> -----
>
> src/tests/master_tests.cpp 1dfe5fd7c33c3c479175aa522fef1956f11f265c
>
>
> Diff: https://reviews.apache.org/r/59578/diff/1/
>
>
> Testing
> -------
>
> `make check`
>
>
> Thanks,
>
> Neil Conway
>
>