----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/17304/#review33104 -----------------------------------------------------------
Ship it! 3rdparty/libprocess/include/process/reap.hpp <https://reviews.apache.org/r/17304/#comment62368> A suggestion: Returns the exit status of the specified process if and only if the process is a direct child and it has not already been reaped. Otherwise, returns None once the process has been reaped elsewhere (or does not exist which is indistinguishable from being reaped elsewhere). 3rdparty/libprocess/include/process/reap.hpp <https://reviews.apache.org/r/17304/#comment62369> // namespace process { 3rdparty/libprocess/src/reap.cpp <https://reviews.apache.org/r/17304/#comment62370> The comment is great and could actually be the comment for process::reap! 3rdparty/libprocess/src/reap.cpp <https://reviews.apache.org/r/17304/#comment62371> We had discussed this back when Yan worked on this code ... but I don't remember what we determined and there isn't a comment. :( Does this mean we might give false positives? What's worse, a false positive or never telling the user about a terminated process? Note that above you made an error be a Failure. 3rdparty/libprocess/src/tests/reap_tests.cpp <https://reviews.apache.org/r/17304/#comment62372> Is this comment still true after MESOS-952 is fixed? 3rdparty/libprocess/src/tests/reap_tests.cpp <https://reviews.apache.org/r/17304/#comment62374> Why is waiting 5 milliseconds sufficient? Seems like if you really wanted to check this you'd EXPECT_DISPATCH on RepearProcess::wait and expect the status to still be pending after Clock::settle. Unless I'm missing something the 5 milliseconds seems like it's misleading the reader. 3rdparty/libprocess/src/tests/reap_tests.cpp <https://reviews.apache.org/r/17304/#comment62375> s/executor/grandchild/ 3rdparty/libprocess/src/tests/reap_tests.cpp <https://reviews.apache.org/r/17304/#comment62376> Maybe a comment as to why you're not ASSERT_SOME(status) and/or checking that status.get().get() is equal to 0? 3rdparty/libprocess/src/tests/reap_tests.cpp <https://reviews.apache.org/r/17304/#comment62373> I know this isn't yours but I'd prefer to not make this a convention, I don't see any problem doing status.get().get() in the next two lines (in the next test too). - Benjamin Hindman On Jan. 29, 2014, 3:39 a.m., Ben Mahler wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/17304/ > ----------------------------------------------------------- > > (Updated Jan. 29, 2014, 3:39 a.m.) > > > Review request for mesos, Benjamin Hindman, Ian Downes, and Jie Yu. > > > Bugs: MESOS-943 > https://issues.apache.org/jira/browse/MESOS-943 > > > Repository: mesos-git > > > Description > ------- > > This is a copy of the Reaper from Mesos, with one key difference: This now > only reaps pids that were monitored explicitly. > > The test needed updating to reflect the fact that we can now retrieve the > exit status for an already exited process. > > This also opens up the opportunity to eliminate the 1 second delay in the > Reaper (using threads or SIGCHLD), but this is a copy of the Mesos Reaper for > now. > > > Diffs > ----- > > 3rdparty/libprocess/Makefile.am 40f01a7b3803696ccca440c8326e1d6d7c377459 > 3rdparty/libprocess/include/process/reap.hpp PRE-CREATION > 3rdparty/libprocess/src/reap.cpp PRE-CREATION > 3rdparty/libprocess/src/tests/reap_tests.cpp PRE-CREATION > > Diff: https://reviews.apache.org/r/17304/diff/ > > > Testing > ------- > > make check > > > Thanks, > > Ben Mahler > >
