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/> >
