----------------------------------------------------------- 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 > >
