----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/8570/#review15556 -----------------------------------------------------------
src/slave/reaper.cpp <https://reviews.apache.org/r/8570/#comment33584> Cool. I like the cleanup, but I still think the long term solution here is to return a Future that gets satisfied once a pid is gone (reaped by us or otherwise). src/slave/reaper.cpp <https://reviews.apache.org/r/8570/#comment33574> Move to just above line 93 please. src/slave/reaper.cpp <https://reviews.apache.org/r/8570/#comment33573> Swap these two lines. src/tests/reaper_tests.cpp <https://reviews.apache.org/r/8570/#comment33575> In other tests, we used 'ASSERT_NE(-1, pipe(pipes));'. While I like the idea of getting more debug info from the PLOG, I also like the idea of only aborting this test case. Does a PASSERT exist? src/tests/reaper_tests.cpp <https://reviews.apache.org/r/8570/#comment33576> I'm guilty of this too, but we really should not treat 'pid' as a boolean, because it is not. We should probably do a general "sweep" through the code and rectify everywhere. src/tests/reaper_tests.cpp <https://reviews.apache.org/r/8570/#comment33579> Ditto above. src/tests/reaper_tests.cpp <https://reviews.apache.org/r/8570/#comment33577> This is a better place for doing something like PLOG, but you'll want to use perror since a child process might have the dreaded glog fork deadlock. Note that you _DO_ want to kill the entire process in this case otherwise you'll have two processes executing test cases, which gets very confusing for the person running tests. src/tests/reaper_tests.cpp <https://reviews.apache.org/r/8570/#comment33580> If you want to be sure, use a pipe. src/tests/reaper_tests.cpp <https://reviews.apache.org/r/8570/#comment33582> Ditto EXIT comment above. src/tests/reaper_tests.cpp <https://reviews.apache.org/r/8570/#comment33583> s/while(1/while (true/ - Benjamin Hindman On Jan. 19, 2013, 1:52 a.m., Vinod Kone wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/8570/ > ----------------------------------------------------------- > > (Updated Jan. 19, 2013, 1:52 a.m.) > > > Review request for mesos, Benjamin Hindman and Ben Mahler. > > > Description > ------- > > Needed this to properly monitor the exit status of re-connected executors, as > they will be parented by INIT. > > > Diffs > ----- > > src/Makefile.am 0ab59b75b2955c532d0833f132bdaffe323838d0 > src/linux/proc.hpp e0825a4a9f9e2763e0c25d7319f220bfe7c7c29c > src/linux/proc.cpp 8a0fc48dc9769df35d682ece477246b2df2fc0d4 > src/slave/cgroups_isolation_module.cpp > 63cefc33cf34eebb82db5d8448b751be8652fa36 > src/slave/lxc_isolation_module.cpp 30cff2a49339bb07030727d30352536a0a22d58c > src/slave/process_based_isolation_module.cpp > 3d50a4b652e4e09dd57e744e408c8fb79ff3fbf5 > src/slave/reaper.hpp b9aa62daa42bdaa736ade43884982529ba3d4bb1 > src/slave/reaper.cpp c0ee4b4c07fd792bcb39455b666808b712eb32c2 > src/tests/master_tests.cpp 948ab5dff34eeba1f3ce593a864ddf282c8b19ed > src/tests/reaper_tests.cpp PRE-CREATION > src/tests/utils.hpp be457117515ee727af101370b26bf9188afb8f45 > third_party/libprocess/Makefile.am 11f8b36d19313f65f1b191a781c6d5ed90130099 > third_party/libprocess/include/stout/proc.hpp PRE-CREATION > > Diff: https://reviews.apache.org/r/8570/diff/ > > > Testing > ------- > > make check > > > Thanks, > > Vinod Kone > >
