----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/8570/#review16540 -----------------------------------------------------------
src/slave/reaper.hpp <https://reviews.apache.org/r/8570/#comment35105> I like this TODO! src/slave/reaper.hpp <https://reviews.apache.org/r/8570/#comment35096> I think these primitives are more clear as: listen(Listener) monitor(pid_t) The external api user shouldn't be thinking about "adding" a pid, it's more that they want the reaper to "monitor" the pid. Likewise with listen, although the verb applies more to the listener itself: Listener.listen(reaper); Because it's not the reaper doing the listening, it's the listener doing the listening. So maybe it's best to leave this one as addListener for now. Certainly once this returns a Future, listen makes sense! reaper.listen(pid_t).onAny(...); src/slave/reaper.cpp <https://reviews.apache.org/r/8570/#comment35106> what's going on with the scoping here..? this loop pid is hiding the above pid? src/slave/reaper.cpp <https://reviews.apache.org/r/8570/#comment35103> Here, you're passing a potentially uninitialized status to the listener! Instead this should notify the listeners with the fact that the status is unknown. Maybe the Future refactoring needs to happen now since it's difficult for you to propagate the error back out to the listeners? src/slave/reaper.cpp <https://reviews.apache.org/r/8570/#comment35107> can you move the pid declaration down to here and do the if check separately from the assignment? Also, shouldn't this be a loop here, until this returns -1, I imagine we'd want to pick up all the exited children here rather than only process 1 per second? i.e. changing this if to a while should be sufficient, right? src/tests/reaper_tests.cpp <https://reviews.apache.org/r/8570/#comment35109> s/In parent process/Still in child process. src/tests/reaper_tests.cpp <https://reviews.apache.org/r/8570/#comment35110> s/(grand) child/grandchild third_party/libprocess/include/stout/proc.hpp <https://reviews.apache.org/r/8570/#comment35104> Wait.. why is this file called proc.hpp? That makes me think of linux/proc.hpp inside mesos, which operates on the linux-only proc pseudo-filesystem. I think for now, we can come up with a different namespace than proc, or you could just stuff this in os.hpp, what do you think? - Ben Mahler On Jan. 24, 2013, 12:17 a.m., Vinod Kone wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/8570/ > ----------------------------------------------------------- > > (Updated Jan. 24, 2013, 12:17 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/sched/sched.cpp f1eeab6f12ee300d77013c6a4ba62ccd7fdb0d1d > 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/slave/solaris_project_isolation_module.cpp > f3b6a68926af34c46873d8de1c9858480f42ef98 > src/tests/cgroups_tests.cpp b219906374764e91f1a5268469ae92dd0fe08e53 > src/tests/master_tests.cpp 948ab5dff34eeba1f3ce593a864ddf282c8b19ed > src/tests/process_spawn.cpp 04e836f8eed7312dbee27e20399e7d0e59df0bc2 > src/tests/reaper_tests.cpp PRE-CREATION > src/tests/script.cpp ebd2ab52e4de2dac744712b5adb1107a33ed29df > 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 > >
