As in, if the user discards the future, the destroy/freeze operation will be aborted?
This sounds fine to me. On Mon, Jul 2, 2012 at 6:11 PM, Jie Yu <[email protected]> wrote: > 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/> >>> >> >> >
