You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mesos.apache.org by Timothy Chen <tn...@apache.org> on 2014/07/15 22:34:15 UTC
Re: Review Request 23443: Re-enable health check tests
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23443/
-----------------------------------------------------------
(Updated July 15, 2014, 8:34 p.m.)
Review request for mesos, Adam B, Ben Mahler, Jie Yu, and Niklas Nielsen.
Summary (updated)
-----------------
Re-enable health check tests
Bugs: MESOS-1533
https://issues.apache.org/jira/browse/MESOS-1533
Repository: mesos-git
Description (updated)
-------
Currently the health check tests are disabled as it was flaky. Part of the reason is that all assertions was EXPECTs instead of ASSERTs so it will continue to execute.
Another issue was it's currently discovering the path to launch health check executable from the command executor main argv[0] path.
However, the correct path is to launch the generated script that does library path resolution, which is the parent of .libs.
Diffs (updated)
-----
src/launcher/executor.cpp a573637
src/tests/health_check_tests.cpp 44711fd
Diff: https://reviews.apache.org/r/23443/diff/
Testing
-------
make check
Thanks,
Timothy Chen
Re: Review Request 23443: Re-enable health check tests
Posted by Mesos ReviewBot <de...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23443/#review47947
-----------------------------------------------------------
Bad patch!
Reviews applied: [23443]
Failed command: ./support/mesos-style.py
Error:
Checking 490 files using filter --filter=-,+build/class,+build/deprecated,+build/endif_comment,+runtime/vlog,+whitespace/blank_line,+whitespace/comma,+whitespace/ending_newline,+whitespace/forcolon,+whitespace/indent,+whitespace/line_length,+whitespace/tab,+whitespace/todo
src/tests/health_check_tests.cpp:78: Lines should be <= 80 characters long [whitespace/line_length] [2]
Total errors found: 1
- Mesos ReviewBot
On July 16, 2014, 8:08 p.m., Timothy Chen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/23443/
> -----------------------------------------------------------
>
> (Updated July 16, 2014, 8:08 p.m.)
>
>
> Review request for mesos, Adam B, Ben Mahler, Jie Yu, and Niklas Nielsen.
>
>
> Bugs: MESOS-1533
> https://issues.apache.org/jira/browse/MESOS-1533
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Currently the health check tests are disabled as it was flaky. Part of the reason is that all assertions was EXPECTs instead of ASSERTs so it will continue to execute.
> Another issue was it's currently discovering the path to launch health check executable from the command executor main argv[0] path.
> However, the correct path is to launch the generated script that does library path resolution, which is the parent of .libs.
>
>
> Diffs
> -----
>
> src/launcher/executor.cpp a573637
> src/tests/health_check_tests.cpp 44711fd
>
> Diff: https://reviews.apache.org/r/23443/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Timothy Chen
>
>
Re: Review Request 23443: Re-enable health check tests
Posted by Mesos ReviewBot <de...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23443/#review47960
-----------------------------------------------------------
Patch looks great!
Reviews applied: [23443]
All tests passed.
- Mesos ReviewBot
On July 16, 2014, 9:36 p.m., Timothy Chen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/23443/
> -----------------------------------------------------------
>
> (Updated July 16, 2014, 9:36 p.m.)
>
>
> Review request for mesos, Adam B, Ben Mahler, Jie Yu, and Niklas Nielsen.
>
>
> Bugs: MESOS-1533
> https://issues.apache.org/jira/browse/MESOS-1533
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Currently the health check tests are disabled as it was flaky. Part of the reason is that all assertions was EXPECTs instead of ASSERTs so it will continue to execute.
> Another issue was it's currently discovering the path to launch health check executable from the command executor main argv[0] path.
> However, the correct path is to launch the generated script that does library path resolution, which is the parent of .libs.
>
>
> Diffs
> -----
>
> src/launcher/executor.cpp a573637
> src/tests/health_check_tests.cpp 44711fd
>
> Diff: https://reviews.apache.org/r/23443/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Timothy Chen
>
>
Re: Review Request 23443: Re-enable health check tests
Posted by Timothy Chen <tn...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23443/
-----------------------------------------------------------
(Updated July 16, 2014, 9:36 p.m.)
Review request for mesos, Adam B, Ben Mahler, Jie Yu, and Niklas Nielsen.
Bugs: MESOS-1533
https://issues.apache.org/jira/browse/MESOS-1533
Repository: mesos-git
Description
-------
Currently the health check tests are disabled as it was flaky. Part of the reason is that all assertions was EXPECTs instead of ASSERTs so it will continue to execute.
Another issue was it's currently discovering the path to launch health check executable from the command executor main argv[0] path.
However, the correct path is to launch the generated script that does library path resolution, which is the parent of .libs.
Diffs (updated)
-----
src/launcher/executor.cpp a573637
src/tests/health_check_tests.cpp 44711fd
Diff: https://reviews.apache.org/r/23443/diff/
Testing
-------
make check
Thanks,
Timothy Chen
Re: Review Request 23443: Re-enable health check tests
Posted by Timothy Chen <tn...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23443/
-----------------------------------------------------------
(Updated July 16, 2014, 8:08 p.m.)
Review request for mesos, Adam B, Ben Mahler, Jie Yu, and Niklas Nielsen.
Bugs: MESOS-1533
https://issues.apache.org/jira/browse/MESOS-1533
Repository: mesos-git
Description
-------
Currently the health check tests are disabled as it was flaky. Part of the reason is that all assertions was EXPECTs instead of ASSERTs so it will continue to execute.
Another issue was it's currently discovering the path to launch health check executable from the command executor main argv[0] path.
However, the correct path is to launch the generated script that does library path resolution, which is the parent of .libs.
Diffs (updated)
-----
src/launcher/executor.cpp a573637
src/tests/health_check_tests.cpp 44711fd
Diff: https://reviews.apache.org/r/23443/diff/
Testing
-------
make check
Thanks,
Timothy Chen
Re: Review Request 23443: Re-enable health check tests
Posted by Timothy Chen <tn...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23443/
-----------------------------------------------------------
(Updated July 16, 2014, 7:19 p.m.)
Review request for mesos, Adam B, Ben Mahler, Jie Yu, and Niklas Nielsen.
Bugs: MESOS-1533
https://issues.apache.org/jira/browse/MESOS-1533
Repository: mesos-git
Description
-------
Currently the health check tests are disabled as it was flaky. Part of the reason is that all assertions was EXPECTs instead of ASSERTs so it will continue to execute.
Another issue was it's currently discovering the path to launch health check executable from the command executor main argv[0] path.
However, the correct path is to launch the generated script that does library path resolution, which is the parent of .libs.
Diffs (updated)
-----
src/launcher/executor.cpp a573637
src/tests/health_check_tests.cpp 44711fd
Diff: https://reviews.apache.org/r/23443/diff/
Testing
-------
make check
Thanks,
Timothy Chen
Re: Review Request 23443: Re-enable health check tests
Posted by Mesos ReviewBot <de...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23443/#review47931
-----------------------------------------------------------
Bad patch!
Reviews applied: [23443]
Failed command: ./support/mesos-style.py
Error:
Checking 490 files using filter --filter=-,+build/class,+build/deprecated,+build/endif_comment,+runtime/vlog,+whitespace/blank_line,+whitespace/comma,+whitespace/ending_newline,+whitespace/forcolon,+whitespace/indent,+whitespace/line_length,+whitespace/tab,+whitespace/todo
src/tests/health_check_tests.cpp:78: Lines should be <= 80 characters long [whitespace/line_length] [2]
Total errors found: 1
- Mesos ReviewBot
On July 16, 2014, 5:30 p.m., Timothy Chen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/23443/
> -----------------------------------------------------------
>
> (Updated July 16, 2014, 5:30 p.m.)
>
>
> Review request for mesos, Adam B, Ben Mahler, Jie Yu, and Niklas Nielsen.
>
>
> Bugs: MESOS-1533
> https://issues.apache.org/jira/browse/MESOS-1533
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Currently the health check tests are disabled as it was flaky. Part of the reason is that all assertions was EXPECTs instead of ASSERTs so it will continue to execute.
> Another issue was it's currently discovering the path to launch health check executable from the command executor main argv[0] path.
> However, the correct path is to launch the generated script that does library path resolution, which is the parent of .libs.
>
>
> Diffs
> -----
>
> src/launcher/executor.cpp a573637
> src/tests/health_check_tests.cpp 44711fd
>
> Diff: https://reviews.apache.org/r/23443/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Timothy Chen
>
>
Re: Review Request 23443: Re-enable health check tests
Posted by Timothy Chen <tn...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23443/
-----------------------------------------------------------
(Updated July 16, 2014, 5:30 p.m.)
Review request for mesos, Adam B, Ben Mahler, Jie Yu, and Niklas Nielsen.
Bugs: MESOS-1533
https://issues.apache.org/jira/browse/MESOS-1533
Repository: mesos-git
Description
-------
Currently the health check tests are disabled as it was flaky. Part of the reason is that all assertions was EXPECTs instead of ASSERTs so it will continue to execute.
Another issue was it's currently discovering the path to launch health check executable from the command executor main argv[0] path.
However, the correct path is to launch the generated script that does library path resolution, which is the parent of .libs.
Diffs (updated)
-----
src/launcher/executor.cpp a573637
src/tests/health_check_tests.cpp 44711fd
Diff: https://reviews.apache.org/r/23443/diff/
Testing
-------
make check
Thanks,
Timothy Chen
Re: Review Request 23443: Re-enable health check tests
Posted by Adam B <ad...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23443/#review47862
-----------------------------------------------------------
LGTM, but I'll let BenH give his final word since he's already had some comments.
Also, I'm still not convinced we need to change all the EXPECTs to ASSERTs. While an ASSERT will fail the test/function faster, it may also skip cleanup code causing a leak.
- Adam B
On July 15, 2014, 1:34 p.m., Timothy Chen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/23443/
> -----------------------------------------------------------
>
> (Updated July 15, 2014, 1:34 p.m.)
>
>
> Review request for mesos, Adam B, Ben Mahler, Jie Yu, and Niklas Nielsen.
>
>
> Bugs: MESOS-1533
> https://issues.apache.org/jira/browse/MESOS-1533
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Currently the health check tests are disabled as it was flaky. Part of the reason is that all assertions was EXPECTs instead of ASSERTs so it will continue to execute.
> Another issue was it's currently discovering the path to launch health check executable from the command executor main argv[0] path.
> However, the correct path is to launch the generated script that does library path resolution, which is the parent of .libs.
>
>
> Diffs
> -----
>
> src/launcher/executor.cpp a573637
> src/tests/health_check_tests.cpp 44711fd
>
> Diff: https://reviews.apache.org/r/23443/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Timothy Chen
>
>
Re: Review Request 23443: Re-enable health check tests
Posted by Timothy Chen <tn...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23443/#review47842
-----------------------------------------------------------
Vinod help me setup a CI job running the health check tests with this fix and it has been passing over 3 times so far (https://builds.apache.org/view/All/job/mesos-tnachen/)
I think it should be good to re-enable again, let me know if anyone thinks different!
- Timothy Chen
On July 15, 2014, 8:34 p.m., Timothy Chen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/23443/
> -----------------------------------------------------------
>
> (Updated July 15, 2014, 8:34 p.m.)
>
>
> Review request for mesos, Adam B, Ben Mahler, Jie Yu, and Niklas Nielsen.
>
>
> Bugs: MESOS-1533
> https://issues.apache.org/jira/browse/MESOS-1533
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Currently the health check tests are disabled as it was flaky. Part of the reason is that all assertions was EXPECTs instead of ASSERTs so it will continue to execute.
> Another issue was it's currently discovering the path to launch health check executable from the command executor main argv[0] path.
> However, the correct path is to launch the generated script that does library path resolution, which is the parent of .libs.
>
>
> Diffs
> -----
>
> src/launcher/executor.cpp a573637
> src/tests/health_check_tests.cpp 44711fd
>
> Diff: https://reviews.apache.org/r/23443/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Timothy Chen
>
>
Re: Review Request 23443: Re-enable health check tests
Posted by Timothy Chen <tn...@apache.org>.
> On July 16, 2014, 4:04 p.m., Benjamin Hindman wrote:
> > src/tests/health_check_tests.cpp, line 79
> > <https://reviews.apache.org/r/23443/diff/4/?file=632794#file632794line79>
> >
> > Please add a newline above and a comment that explains why we need to do this and why we're calling this MESOS_LAUNCHER_DIR. In fact, I was expecting the slave to always set this environment variable (like your old patch did originally), but I think this is fine for now. Also, minor nit: s/Environment_Variable* var/Environment::Variable* variable/. Thanks Tim!
Sounds good, I just reverted the EXPECT ASSERT changes and addressed your comment.
The tests has been passing all night so far. Thanks!
- Timothy
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23443/#review47891
-----------------------------------------------------------
On July 16, 2014, 5:30 p.m., Timothy Chen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/23443/
> -----------------------------------------------------------
>
> (Updated July 16, 2014, 5:30 p.m.)
>
>
> Review request for mesos, Adam B, Ben Mahler, Jie Yu, and Niklas Nielsen.
>
>
> Bugs: MESOS-1533
> https://issues.apache.org/jira/browse/MESOS-1533
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Currently the health check tests are disabled as it was flaky. Part of the reason is that all assertions was EXPECTs instead of ASSERTs so it will continue to execute.
> Another issue was it's currently discovering the path to launch health check executable from the command executor main argv[0] path.
> However, the correct path is to launch the generated script that does library path resolution, which is the parent of .libs.
>
>
> Diffs
> -----
>
> src/launcher/executor.cpp a573637
> src/tests/health_check_tests.cpp 44711fd
>
> Diff: https://reviews.apache.org/r/23443/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Timothy Chen
>
>
Re: Review Request 23443: Re-enable health check tests
Posted by Benjamin Hindman <be...@berkeley.edu>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23443/#review47891
-----------------------------------------------------------
Ship it!
If these tests are now passing let's revert the EXPECT to ASSERT. Also, see my comment below. Otherwise, LGTM.
src/tests/health_check_tests.cpp
<https://reviews.apache.org/r/23443/#comment84118>
Please add a newline above and a comment that explains why we need to do this and why we're calling this MESOS_LAUNCHER_DIR. In fact, I was expecting the slave to always set this environment variable (like your old patch did originally), but I think this is fine for now. Also, minor nit: s/Environment_Variable* var/Environment::Variable* variable/. Thanks Tim!
- Benjamin Hindman
On July 15, 2014, 8:34 p.m., Timothy Chen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/23443/
> -----------------------------------------------------------
>
> (Updated July 15, 2014, 8:34 p.m.)
>
>
> Review request for mesos, Adam B, Ben Mahler, Jie Yu, and Niklas Nielsen.
>
>
> Bugs: MESOS-1533
> https://issues.apache.org/jira/browse/MESOS-1533
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Currently the health check tests are disabled as it was flaky. Part of the reason is that all assertions was EXPECTs instead of ASSERTs so it will continue to execute.
> Another issue was it's currently discovering the path to launch health check executable from the command executor main argv[0] path.
> However, the correct path is to launch the generated script that does library path resolution, which is the parent of .libs.
>
>
> Diffs
> -----
>
> src/launcher/executor.cpp a573637
> src/tests/health_check_tests.cpp 44711fd
>
> Diff: https://reviews.apache.org/r/23443/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Timothy Chen
>
>