You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mesos.apache.org by Craig Hansen-Sturm <cr...@mesosphere.io> on 2014/08/05 01:23:28 UTC
Review Request 24270: New MultiChildProcess test added to reap_tests.cpp
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/24270/
-----------------------------------------------------------
Review request for mesos and Ian Downes.
Bugs: MESOS-1669
https://issues.apache.org/jira/browse/MESOS-1669
Repository: mesos-git
Description
-------
New test added which serves as a performance test for reaping. This test forks N children (default value is 4), kills these children, and then uses process::collect to collect all status results. This is a simple generalization of the existing ChildProcess test, except that it is realtime (as required for profiling), e.g., no clock manipulation is performed. This test is also useful for stressing the collect.hpp interfaces.
Diffs
-----
3rdparty/libprocess/src/tests/reap_tests.cpp a18d54c
Diff: https://reviews.apache.org/r/24270/diff/
Testing
-------
make check
Thanks,
Craig Hansen-Sturm
Re: Review Request 24270: New MultiChildProcess test added to reap_tests.cpp
Posted by Mesos ReviewBot <de...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/24270/#review49857
-----------------------------------------------------------
Patch looks great!
Reviews applied: [24270]
All tests passed.
- Mesos ReviewBot
On Aug. 5, 2014, 11:33 p.m., Craig Hansen-Sturm wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/24270/
> -----------------------------------------------------------
>
> (Updated Aug. 5, 2014, 11:33 p.m.)
>
>
> Review request for mesos and Ian Downes.
>
>
> Bugs: MESOS-1669
> https://issues.apache.org/jira/browse/MESOS-1669
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> New test added which serves as a performance test for reaping. This test forks N children (default value is 4), kills these children, and then uses process::collect to collect all status results. This is a simple generalization of the existing ChildProcess test, except that it is realtime (as required for profiling), e.g., no clock manipulation is performed. This test is also useful for stressing the collect.hpp interfaces.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/src/tests/reap_tests.cpp a18d54c
>
> Diff: https://reviews.apache.org/r/24270/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Craig Hansen-Sturm
>
>
Re: Review Request 24270: New MultiChildProcess test added to reap_tests.cpp
Posted by Craig Hansen-Sturm <cr...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/24270/
-----------------------------------------------------------
(Updated Aug. 5, 2014, 11:33 p.m.)
Review request for mesos and Ian Downes.
Changes
-------
Addresses Ian's review comments.
Bugs: MESOS-1669
https://issues.apache.org/jira/browse/MESOS-1669
Repository: mesos-git
Description
-------
New test added which serves as a performance test for reaping. This test forks N children (default value is 4), kills these children, and then uses process::collect to collect all status results. This is a simple generalization of the existing ChildProcess test, except that it is realtime (as required for profiling), e.g., no clock manipulation is performed. This test is also useful for stressing the collect.hpp interfaces.
Diffs (updated)
-----
3rdparty/libprocess/src/tests/reap_tests.cpp a18d54c
Diff: https://reviews.apache.org/r/24270/diff/
Testing
-------
make check
Thanks,
Craig Hansen-Sturm
Re: Review Request 24270: New MultiChildProcess test added to reap_tests.cpp
Posted by Timothy Chen <tn...@apache.org>.
> On Aug. 5, 2014, 12:30 a.m., Ian Downes wrote:
> > I'm not 100% clear from the description what this is doing - does it test correctness in anyway or is it really to be used under profiling to test the performance of the reaper? If it's the latter, then perhaps this should be standalone benchmark that steps through # of child pids?
>
> Craig Hansen-Sturm wrote:
> This is meant for benchmarking; however, it is has dual purpose, and it wasn't clear how to integrate this type of benchmark. Ideally, we'd want to continuously vary the pid count and, for this benchmark, the reap wait time. The wait time isn't programmatically exposed, but the question I have for you, is do we have a standard mechanism for exposing "endpoints" such as this ? Create a new internal interface ?
>
> Finally, ideally, this test would make use of an external profiling api - so that we can start/stop the sampling periods at test entry.
>
> I'm hoping GTEST has a lot of these hooks, but want to hear your ideas.
Just came to my mind that we already have a make bench call that runs BENCHMARK tests I believe. Can this test then fold under benchmark with a BENCHMARK tag as well?
- Timothy
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/24270/#review49541
-----------------------------------------------------------
On Aug. 4, 2014, 11:23 p.m., Craig Hansen-Sturm wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/24270/
> -----------------------------------------------------------
>
> (Updated Aug. 4, 2014, 11:23 p.m.)
>
>
> Review request for mesos and Ian Downes.
>
>
> Bugs: MESOS-1669
> https://issues.apache.org/jira/browse/MESOS-1669
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> New test added which serves as a performance test for reaping. This test forks N children (default value is 4), kills these children, and then uses process::collect to collect all status results. This is a simple generalization of the existing ChildProcess test, except that it is realtime (as required for profiling), e.g., no clock manipulation is performed. This test is also useful for stressing the collect.hpp interfaces.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/src/tests/reap_tests.cpp a18d54c
>
> Diff: https://reviews.apache.org/r/24270/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Craig Hansen-Sturm
>
>
Re: Review Request 24270: New MultiChildProcess test added to reap_tests.cpp
Posted by Craig Hansen-Sturm <cr...@mesosphere.io>.
> On Aug. 5, 2014, 12:30 a.m., Ian Downes wrote:
> > 3rdparty/libprocess/src/tests/reap_tests.cpp, lines 154-158
> > <https://reviews.apache.org/r/24270/diff/2/?file=651304#file651304line154>
> >
> > Why not merge these loops? i.e., reap the pid then sigkill it immediately, then you can drop childPid.
The intent here was the get as much "stacking" of notifications as possible. e.g., create and reap all child pids, let the new processes settle (which I have not done), then kill the children. I should add a wait completion of some kind prior to the kill.
I believe that this creates too many races if we create/reap/kill each pid serially - e.g., this is about generating worst-case performance in the reap wait loop.
- Craig
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/24270/#review49541
-----------------------------------------------------------
On Aug. 4, 2014, 11:23 p.m., Craig Hansen-Sturm wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/24270/
> -----------------------------------------------------------
>
> (Updated Aug. 4, 2014, 11:23 p.m.)
>
>
> Review request for mesos and Ian Downes.
>
>
> Bugs: MESOS-1669
> https://issues.apache.org/jira/browse/MESOS-1669
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> New test added which serves as a performance test for reaping. This test forks N children (default value is 4), kills these children, and then uses process::collect to collect all status results. This is a simple generalization of the existing ChildProcess test, except that it is realtime (as required for profiling), e.g., no clock manipulation is performed. This test is also useful for stressing the collect.hpp interfaces.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/src/tests/reap_tests.cpp a18d54c
>
> Diff: https://reviews.apache.org/r/24270/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Craig Hansen-Sturm
>
>
Re: Review Request 24270: New MultiChildProcess test added to reap_tests.cpp
Posted by Craig Hansen-Sturm <cr...@mesosphere.io>.
> On Aug. 5, 2014, 12:30 a.m., Ian Downes wrote:
> > I'm not 100% clear from the description what this is doing - does it test correctness in anyway or is it really to be used under profiling to test the performance of the reaper? If it's the latter, then perhaps this should be standalone benchmark that steps through # of child pids?
This is meant for benchmarking; however, it is has dual purpose, and it wasn't clear how to integrate this type of benchmark. Ideally, we'd want to continuously vary the pid count and, for this benchmark, the reap wait time. The wait time isn't programmatically exposed, but the question I have for you, is do we have a standard mechanism for exposing "endpoints" such as this ? Create a new internal interface ?
Finally, ideally, this test would make use of an external profiling api - so that we can start/stop the sampling periods at test entry.
I'm hoping GTEST has a lot of these hooks, but want to hear your ideas.
- Craig
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/24270/#review49541
-----------------------------------------------------------
On Aug. 4, 2014, 11:23 p.m., Craig Hansen-Sturm wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/24270/
> -----------------------------------------------------------
>
> (Updated Aug. 4, 2014, 11:23 p.m.)
>
>
> Review request for mesos and Ian Downes.
>
>
> Bugs: MESOS-1669
> https://issues.apache.org/jira/browse/MESOS-1669
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> New test added which serves as a performance test for reaping. This test forks N children (default value is 4), kills these children, and then uses process::collect to collect all status results. This is a simple generalization of the existing ChildProcess test, except that it is realtime (as required for profiling), e.g., no clock manipulation is performed. This test is also useful for stressing the collect.hpp interfaces.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/src/tests/reap_tests.cpp a18d54c
>
> Diff: https://reviews.apache.org/r/24270/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Craig Hansen-Sturm
>
>
Re: Review Request 24270: New MultiChildProcess test added to reap_tests.cpp
Posted by Ian Downes <ia...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/24270/#review49541
-----------------------------------------------------------
I'm not 100% clear from the description what this is doing - does it test correctness in anyway or is it really to be used under profiling to test the performance of the reaper? If it's the latter, then perhaps this should be standalone benchmark that steps through # of child pids?
3rdparty/libprocess/src/tests/reap_tests.cpp
<https://reviews.apache.org/r/24270/#comment86651>
Bring in whatever you need from std.
3rdparty/libprocess/src/tests/reap_tests.cpp
<https://reviews.apache.org/r/24270/#comment86655>
We do declarations where they're used, not upfront.
3rdparty/libprocess/src/tests/reap_tests.cpp
<https://reviews.apache.org/r/24270/#comment86652>
> > >
3rdparty/libprocess/src/tests/reap_tests.cpp
<https://reviews.apache.org/r/24270/#comment86660>
We generally pluralize variables when appropriate, e.g., childStatuses.
3rdparty/libprocess/src/tests/reap_tests.cpp
<https://reviews.apache.org/r/24270/#comment86654>
Why not merge these loops? i.e., reap the pid then sigkill it immediately, then you can drop childPid.
3rdparty/libprocess/src/tests/reap_tests.cpp
<https://reviews.apache.org/r/24270/#comment86653>
Use foreach (pid_t pid, childPids) {}
from stout/foreach.hpp and also friends like foreachpair, ...
3rdparty/libprocess/src/tests/reap_tests.cpp
<https://reviews.apache.org/r/24270/#comment86656>
just use AWAIT_READY(collect(childStatus))
Aside, why do you want to collect them all at the end? Why not AWAIT_READY(process::reap(childPid)) in the loop where you fork each child?
I presume this is all because you really want to profile the reaper using this test?
3rdparty/libprocess/src/tests/reap_tests.cpp
<https://reviews.apache.org/r/24270/#comment86657>
collect() will return a failure if any of the futures fail so this isn't necessary.
3rdparty/libprocess/src/tests/reap_tests.cpp
<https://reviews.apache.org/r/24270/#comment86658>
foreach
3rdparty/libprocess/src/tests/reap_tests.cpp
<https://reviews.apache.org/r/24270/#comment86661>
not necessary with foreach
3rdparty/libprocess/src/tests/reap_tests.cpp
<https://reviews.apache.org/r/24270/#comment86659>
We generally use EXPECT_ when possible so the test continues
- Ian Downes
On Aug. 4, 2014, 4:23 p.m., Craig Hansen-Sturm wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/24270/
> -----------------------------------------------------------
>
> (Updated Aug. 4, 2014, 4:23 p.m.)
>
>
> Review request for mesos and Ian Downes.
>
>
> Bugs: MESOS-1669
> https://issues.apache.org/jira/browse/MESOS-1669
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> New test added which serves as a performance test for reaping. This test forks N children (default value is 4), kills these children, and then uses process::collect to collect all status results. This is a simple generalization of the existing ChildProcess test, except that it is realtime (as required for profiling), e.g., no clock manipulation is performed. This test is also useful for stressing the collect.hpp interfaces.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/src/tests/reap_tests.cpp a18d54c
>
> Diff: https://reviews.apache.org/r/24270/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Craig Hansen-Sturm
>
>