Ben, I plan to remove the timeout parameter and let user decide when to cancel the destroy (or freeze) operation.
What do you think? - Jie On Mon, Jul 2, 2012 at 5:34 PM, Jie Yu <[email protected]> wrote: > Ben, > > This patch depends on the freezer patch. Could you please review the > freezer utils patch first? Thanks! (I will address the comments in this > review anyway). > > https://reviews.apache.org/r/5401/ > > - Jie > > > On Sun, Jul 1, 2012 at 10:18 PM, Benjamin Hindman <[email protected]>wrote: > >> This is an automatically generated e-mail. To reply, visit: >> https://reviews.apache.org/r/5402/ >> >> src/linux/cgroups.hpp<https://reviews.apache.org/r/5402/diff/5/?file=115746#file115746line169> >> (Diff >> revision 5) >> >> Try<process::Future<bool> > destroyCgroup( >> >> 169 >> >> Option<double> timeout = Option<double>::none()); >> >> const & for the Option<double>. >> >> >> >> src/linux/cgroups.hpp<https://reviews.apache.org/r/5402/diff/5/?file=115746#file115746line273> >> (Diff >> revision 5) >> >> Try<process::Future<bool> > killTasks( >> >> 273 >> >> Option<double> timeout = Option<double>::none()); >> >> Ditto. >> >> >> >> src/linux/cgroups.cpp<https://reviews.apache.org/r/5402/diff/5/?file=115747#file115747line601> >> (Diff >> revision 5) >> >> public: >> >> 601 >> >> ~CgroupDestroyer() {} >> >> virtual. >> >> >> >> src/linux/cgroups.cpp<https://reviews.apache.org/r/5402/diff/5/?file=115747#file115747line603> >> (Diff >> revision 5) >> >> public: >> >> 603 >> >> void initialize() >> >> This method is also virtual -- also, any reason not to keep this protected? >> >> >> >> src/linux/cgroups.cpp<https://reviews.apache.org/r/5402/diff/5/?file=115747#file115747line606> >> (Diff >> revision 5) >> >> public: >> >> 606 >> >> state().onDiscarded(process::defer(this, &CgroupDestroyer::stop)); >> >> Just like the 'EventListener', this should be a call to terminate. >> >> >> >> src/linux/cgroups.cpp<https://reviews.apache.org/r/5402/diff/5/?file=115747#file115747line613> >> (Diff >> revision 5) >> >> public: >> >> 613 >> >> void start(Option<double> timeout) >> >> Why not pass timeout into the constructor and put all this logic in >> 'initialize'? >> >> >> >> src/linux/cgroups.cpp<https://reviews.apache.org/r/5402/diff/5/?file=115747#file115747line615> >> (Diff >> revision 5) >> >> public: >> >> 615 >> >> if (index >= cgroups.size()){ >> >> Rather than keep this state, this is a great opportunity to fire off the >> 'killTasks' requests in parallel, then do a 'collect' on the futures! >> >> >> >> src/linux/cgroups.cpp<https://reviews.apache.org/r/5402/diff/5/?file=115747#file115747line617> >> (Diff >> revision 5) >> >> public: >> >> 617 >> >> process::dispatch(this, &CgroupDestroyer::stop); >> >> terminate >> >> >> >> src/linux/cgroups.cpp<https://reviews.apache.org/r/5402/diff/5/?file=115747#file115747line627> >> (Diff >> revision 5) >> >> public: >> >> 627 >> >> process::dispatch(this, &CgroupDestroyer::stop); >> >> terminate >> >> >> >> src/linux/cgroups.cpp<https://reviews.apache.org/r/5402/diff/5/?file=115747#file115747line642> >> (Diff >> revision 5) >> >> public: >> >> 642 >> >> Option<double> timeout) >> >> const & >> >> >> >> src/linux/cgroups.cpp<https://reviews.apache.org/r/5402/diff/5/?file=115747#file115747line648> >> (Diff >> revision 5) >> >> public: >> >> 648 >> >> process::dispatch(this, &CgroupDestroyer::stop); >> >> terminate >> >> >> >> src/linux/cgroups.cpp<https://reviews.apache.org/r/5402/diff/5/?file=115747#file115747line655> >> (Diff >> revision 5) >> >> public: >> >> 655 >> >> process::dispatch(this, &CgroupDestroyer::stop); >> >> terminate >> >> >> >> src/linux/cgroups.cpp<https://reviews.apache.org/r/5402/diff/5/?file=115747#file115747line659> >> (Diff >> revision 5) >> >> public: >> >> 659 >> >> void stop() >> >> This function can be removed. >> >> >> >> src/linux/cgroups.cpp<https://reviews.apache.org/r/5402/diff/5/?file=115747#file115747line674> >> (Diff >> revision 5) >> >> public: >> >> 674 >> >> Try<process::Future<bool> > destroyCgroup(const std::string& hierarchy, >> >> Just return the future, no need for the Try too. >> >> >> >> src/linux/cgroups.cpp<https://reviews.apache.org/r/5402/diff/5/?file=115747#file115747line705> >> (Diff >> revision 5) >> >> public: >> >> 705 >> >> process::dispatch(destroyer, &internal::CgroupDestroyer::start, timeout); >> >> See comment above, this extra dispatch can be removed. >> >> >> - Benjamin >> >> On June 21st, 2012, 7:42 p.m., Jie Yu wrote: >> Review request for mesos, Benjamin Hindman and Vinod Kone. >> By Jie Yu. >> >> *Updated June 21, 2012, 7:42 p.m.* >> Description >> >> This patch leverages the freezer subsystem in cgroups to kill all the >> processes in a cgroup atomically. >> >> The main idea is to freeze all the processes in a cgroup first, then send >> kill signal to all the proceses. This avoids the need of walking the proc >> process tree to kill all processes associated with an executor. In fact, the >> original killtree solution assumes that the user processes haven't blocked >> the SIGSTOP signal, which may not be true in some cases. >> >> Testing >> >> On Linux machine, make check. >> >> Diffs >> >> - src/linux/cgroups.hpp (PRE-CREATION) >> - src/linux/cgroups.cpp (PRE-CREATION) >> - src/tests/cgroups_tests.cpp (PRE-CREATION) >> >> View Diff <https://reviews.apache.org/r/5402/diff/> >> > >
