You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Alexander Rukletsov <ru...@gmail.com> on 2016/03/18 18:17:45 UTC
Review Request 45040: Added a test for task's kill policy.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45040/
-----------------------------------------------------------
Review request for mesos and Ben Mahler.
Bugs: MESOS-4909
https://issues.apache.org/jira/browse/MESOS-4909
Repository: mesos
Description
-------
See summary.
Diffs
-----
src/tests/slave_tests.cpp ea1d776077bf638885db8421194aa4427c772169
Diff: https://reviews.apache.org/r/45040/diff/
Testing
-------
On Mac OS 10.104:
`make check`
`GTEST_FILTER="*TaskKillPolicy*" ./bin/mesos-tests.sh --gtest_repeat=100 --gtest_break_on_failure`
Thanks,
Alexander Rukletsov
Re: Review Request 45040: Added a test for task's kill policy.
Posted by Alexander Rukletsov <ru...@gmail.com>.
> On March 19, 2016, 1:35 a.m., Ben Mahler wrote:
> > Thanks for the test! Although I wouldn't want to ship this unless you show some manual testing in the 'testing done' section, since this test doesn't exercise the functionality :)
I think since we pulled up half of the test into a separate test, we don't need this one any more. I'll discard the review request.
> On March 19, 2016, 1:35 a.m., Ben Mahler wrote:
> > src/tests/slave_tests.cpp, lines 3419-3424
> > <https://reviews.apache.org/r/45040/diff/1/?file=1306241#file1306241line3419>
> >
> > You won't need to bother with this once we pull out the validation test, right?
Correct.
> On March 19, 2016, 1:35 a.m., Ben Mahler wrote:
> > src/tests/slave_tests.cpp, lines 3442-3444
> > <https://reviews.apache.org/r/45040/diff/1/?file=1306241#file1306241line3442>
> >
> > You shouldn't need to do this once the validation test is pulled out, right?
Right.
> On March 19, 2016, 1:35 a.m., Ben Mahler wrote:
> > src/tests/slave_tests.cpp, line 3482
> > <https://reviews.apache.org/r/45040/diff/1/?file=1306241#file1306241line3482>
> >
> > Why the resume here?
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.
- Alexander
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45040/#review124343
-----------------------------------------------------------
On March 19, 2016, 10:40 a.m., Alexander Rukletsov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45040/
> -----------------------------------------------------------
>
> (Updated March 19, 2016, 10:40 a.m.)
>
>
> Review request for mesos and Ben Mahler.
>
>
> Bugs: MESOS-4909
> https://issues.apache.org/jira/browse/MESOS-4909
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> src/tests/slave_tests.cpp ea1d776077bf638885db8421194aa4427c772169
>
> Diff: https://reviews.apache.org/r/45040/diff/
>
>
> Testing
> -------
>
> On Mac OS 10.10.4:
> `make check`
> `GTEST_FILTER="*TaskKillPolicy*" ./bin/mesos-tests.sh --gtest_repeat=100 --gtest_break_on_failure`
>
>
> Thanks,
>
> Alexander Rukletsov
>
>
Re: Review Request 45040: Added a test for task's kill policy.
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45040/#review124343
-----------------------------------------------------------
Thanks for the test! Although I wouldn't want to ship this unless you show some manual testing in the 'testing done' section, since this test doesn't exercise the functionality :)
src/tests/slave_tests.cpp (line 3372)
<https://reviews.apache.org/r/45040/#comment186842>
No need for the NOTE (per the previous test review feedback).
src/tests/slave_tests.cpp (lines 3393 - 3431)
<https://reviews.apache.org/r/45040/#comment186826>
Per the feedback from the other test review, let's pull this up into a test within master_validation_tests.cpp. It can be a trivial unit test since the validation function is stateless :)
src/tests/slave_tests.cpp (lines 3419 - 3424)
<https://reviews.apache.org/r/45040/#comment186841>
You won't need to bother with this once we pull out the validation test, right?
src/tests/slave_tests.cpp (lines 3433 - 3477)
<https://reviews.apache.org/r/45040/#comment186835>
Once you pull out the validation test you won't need this scope anymore.
This is just testing that we don't mutate the grace period? Can you add a TODO to the top of this test to mention that we should also actually test that it does what it is supposed to do?
Did you manually test this? I wouldn't feel comfortable shipping this unless you did, since this test doesn't cover the signal escalation behavior :)
src/tests/slave_tests.cpp (lines 3442 - 3444)
<https://reviews.apache.org/r/45040/#comment186840>
You shouldn't need to do this once the validation test is pulled out, right?
src/tests/slave_tests.cpp (lines 3466 - 3476)
<https://reviews.apache.org/r/45040/#comment186838>
It doesn't look like you need to bother sending a status update here, just use FutureArg instead of SaveArg and wait for the argument to arrive. In general we try to avoid SaveArg since it doesn't indicate when the value arrives.
src/tests/slave_tests.cpp (line 3474)
<https://reviews.apache.org/r/45040/#comment186836>
Please sweep for s/.get()./->/
src/tests/slave_tests.cpp (line 3482)
<https://reviews.apache.org/r/45040/#comment186837>
Why the resume here?
- Ben Mahler
On March 18, 2016, 5:17 p.m., Alexander Rukletsov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45040/
> -----------------------------------------------------------
>
> (Updated March 18, 2016, 5:17 p.m.)
>
>
> Review request for mesos and Ben Mahler.
>
>
> Bugs: MESOS-4909
> https://issues.apache.org/jira/browse/MESOS-4909
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> src/tests/slave_tests.cpp ea1d776077bf638885db8421194aa4427c772169
>
> Diff: https://reviews.apache.org/r/45040/diff/
>
>
> Testing
> -------
>
> On Mac OS 10.104:
> `make check`
> `GTEST_FILTER="*TaskKillPolicy*" ./bin/mesos-tests.sh --gtest_repeat=100 --gtest_break_on_failure`
>
>
> Thanks,
>
> Alexander Rukletsov
>
>
Re: Review Request 45040: Added a test for task's kill policy.
Posted by Alexander Rukletsov <ru...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45040/
-----------------------------------------------------------
(Updated March 23, 2016, 11:24 p.m.)
Review request for mesos and Ben Mahler.
Bugs: MESOS-4909
https://issues.apache.org/jira/browse/MESOS-4909
Repository: mesos
Description
-------
See summary.
Diffs
-----
src/tests/slave_tests.cpp ea1d776077bf638885db8421194aa4427c772169
Diff: https://reviews.apache.org/r/45040/diff/
Testing
-------
On Mac OS 10.10.4:
`make check`
`GTEST_FILTER="*TaskKillPolicy*" ./bin/mesos-tests.sh --gtest_repeat=100 --gtest_break_on_failure`
Thanks,
Alexander Rukletsov
Re: Review Request 45040: Added a test for task's kill policy.
Posted by Alexander Rukletsov <ru...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45040/
-----------------------------------------------------------
(Updated March 19, 2016, 10:40 a.m.)
Review request for mesos and Ben Mahler.
Bugs: MESOS-4909
https://issues.apache.org/jira/browse/MESOS-4909
Repository: mesos
Description
-------
See summary.
Diffs
-----
src/tests/slave_tests.cpp ea1d776077bf638885db8421194aa4427c772169
Diff: https://reviews.apache.org/r/45040/diff/
Testing (updated)
-------
On Mac OS 10.10.4:
`make check`
`GTEST_FILTER="*TaskKillPolicy*" ./bin/mesos-tests.sh --gtest_repeat=100 --gtest_break_on_failure`
Thanks,
Alexander Rukletsov