Yes. In that case, timeout is actually controlled by its caller. - Jie
On Tue, Jul 3, 2012 at 8:53 AM, Benjamin Hindman <[email protected]> wrote: > 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/> >>>> >>> >>> >> >
