> On Jan. 29, 2014, 7:03 a.m., Benjamin Hindman wrote: > > 3rdparty/libprocess/src/reap.cpp, line 79 > > <https://reviews.apache.org/r/17304/diff/3/?file=453443#file453443line79> > > > > 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.
Yes good catch, I've updated this to be considered a failed future. > On Jan. 29, 2014, 7:03 a.m., Benjamin Hindman wrote: > > 3rdparty/libprocess/src/tests/reap_tests.cpp, line 94 > > <https://reviews.apache.org/r/17304/diff/3/?file=453444#file453444line94> > > > > Maybe a comment as to why you're not ASSERT_SOME(status) and/or > > checking that status.get().get() is equal to 0? Added the asserts. > On Jan. 29, 2014, 7:03 a.m., Benjamin Hindman wrote: > > 3rdparty/libprocess/src/tests/reap_tests.cpp, line 130 > > <https://reviews.apache.org/r/17304/diff/3/?file=453444#file453444line130> > > > > 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). Unfortunately it doesn't compile due to the way the macros were implemented: ../../../3rdparty/libprocess/src/tests/reap_tests.cpp:88: error: lvalue required as unary ‘&’ operand ../../../3rdparty/libprocess/src/tests/reap_tests.cpp:89: error: lvalue required as unary ‘&’ operand > On Jan. 29, 2014, 7:03 a.m., Benjamin Hindman wrote: > > 3rdparty/libprocess/src/tests/reap_tests.cpp, lines 62-64 > > <https://reviews.apache.org/r/17304/diff/3/?file=453444#file453444line62> > > > > 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. It's technically not sufficient, I added it to compensate for the fact that the previous code was expecting on dispatches, but I don't think it is necessary for this test so I've removed it. - Ben ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/17304/#review33104 ----------------------------------------------------------- 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 > >
