----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/11849/#review21951 -----------------------------------------------------------
Ship it! This is pretty darn clean and simple! Happy to see killtree.sh go away, but we should consider updating the Stout Makefile to build a 'killtree' binary to enable other people to easily use killtree from the command line! In fact, it might be nice to ship/install something like that with Mesos. Also, I'm a big +1 for capturing output for debugging purposes. Given the nature of this being in stout, what about taking a std::ostream that this function uses (similar to os::shell) and then passing '&LOG(INFO)' when we use this in ProcessIsolator? I'd love to see that before this is committed. 3rdparty/libprocess/3rdparty/stout/include/stout/os/killtree.hpp <https://reviews.apache.org/r/11849/#comment45271> This big comment should go down just before we actually send the signal. 3rdparty/libprocess/3rdparty/stout/include/stout/os/killtree.hpp <https://reviews.apache.org/r/11849/#comment45273> I'm okay with s/_pid/pid/ if you like. ;) 3rdparty/libprocess/3rdparty/stout/include/stout/os/killtree.hpp <https://reviews.apache.org/r/11849/#comment45272> I think I'm a +1 for 'visited'. Basically this is preorder BFS where the "visiting" component is to do the stop, so knowing that you've "visited" implies that you've already sent a stop. Also, in a BFS you wouldn't have to check if you already visited (you'd just add all the children), so I think a comment here explaining that we need to check if the process has been already visited since we might have already added the process (and thus visited it) via the group or session lookups. 3rdparty/libprocess/3rdparty/stout/include/stout/os/killtree.hpp <https://reviews.apache.org/r/11849/#comment45274> Again, okay with s/_pid/pid/ (it's interesting that you're okay with the hiding here but not above). ;) 3rdparty/libprocess/3rdparty/stout/include/stout/os/killtree.hpp <https://reviews.apache.org/r/11849/#comment45275> s/we now/we/ 3rdparty/libprocess/3rdparty/stout/include/stout/os/killtree.hpp <https://reviews.apache.org/r/11849/#comment45276> s/_pid/pid/ ;) 3rdparty/libprocess/3rdparty/stout/include/stout/os/killtree.hpp <https://reviews.apache.org/r/11849/#comment45277> s/_pid/pid/ ;) 3rdparty/libprocess/3rdparty/stout/include/stout/os/killtree.hpp <https://reviews.apache.org/r/11849/#comment45278> What are the possible errors? Is this forward looking? 3rdparty/libprocess/3rdparty/stout/tests/os_tests.cpp <https://reviews.apache.org/r/11849/#comment45267> +1. - Benjamin Hindman On June 12, 2013, 7:49 p.m., Ben Mahler wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/11849/ > ----------------------------------------------------------- > > (Updated June 12, 2013, 7:49 p.m.) > > > Review request for mesos, Benjamin Hindman and Vinod Kone. > > > Description > ------- > > For how killtree will be used, see the killtree.sh script in mesos. > > This version is actually more robust than killtree.sh, as it first stops the > entire tree, _then_ issues the signal. > Currently, killtree.sh stops, signals, and continues each process in > isolation. > > > Diffs > ----- > > 3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp > 1b3fb47d7567b5467fef2a2bb15d5c4a2ea42aa5 > 3rdparty/libprocess/3rdparty/stout/include/stout/os/killtree.hpp > PRE-CREATION > 3rdparty/libprocess/3rdparty/stout/tests/os_tests.cpp > 047778d05ebbbefd85e4a163dbb6ab8445edfb7f > > Diff: https://reviews.apache.org/r/11849/diff/ > > > Testing > ------- > > Added a test, however this test can be _significantly_ cleaned up were we to > create an abstraction for generating process trees! > > > Thanks, > > Ben Mahler > >
