----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/20818/#review42977 -----------------------------------------------------------
src/linux/cgroups.hpp <https://reviews.apache.org/r/20818/#comment76956> It's probably fine for this review. But in the future, we should try to push timeout to higher level if possible. For example, one can discard the future returned by 'destroy' to cancel the operation and use future.after to setup timeout: destroy(...).after(Seconds(60), [](...) { future.discard(); return Failure(...); }); src/linux/cgroups.hpp <https://reviews.apache.org/r/20818/#comment76958> Use 'Timeout' here, instead of Duration, given that Freezer uses Timeout? src/linux/cgroups.cpp <https://reviews.apache.org/r/20818/#comment76955> This sequence of operations are introduced in this commit. Probably you should contact the author to see if your current logic is OK. If yes, please add some comments to explain why it is OK. commit 77db3cb32f9d25656b86607f6f241b2303dbd982 Author: Brenden Matthews <brenden.matth...@airbnb.com> Date: Fri May 10 16:00:15 2013 -0700 Changed cgroups killTasks() sequence. The sequence is as follows: stop -> kill -> empty -> freeze -> kill -> thaw -> empty We also now ignore ESRCH errors from kill() in cgroups::kill(). Review: https://reviews.apache.org/r/11131 src/linux/cgroups.cpp <https://reviews.apache.org/r/20818/#comment76959> So the 'timeout' specified by the user is not the total timeout for the operation? src/linux/cgroups.cpp <https://reviews.apache.org/r/20818/#comment76960> Do you need to discard the future here: future.discard()? src/linux/cgroups.cpp <https://reviews.apache.org/r/20818/#comment76961> Ditto. - Jie Yu On May 5, 2014, 8:16 p.m., Ian Downes wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/20818/ > ----------------------------------------------------------- > > (Updated May 5, 2014, 8:16 p.m.) > > > Review request for mesos, Benjamin Hindman, Ben Mahler, Chi Zhang, Jie Yu, > and Vinod Kone. > > > Bugs: MESOS-759 and MESOS-976 > https://issues.apache.org/jira/browse/MESOS-759 > https://issues.apache.org/jira/browse/MESOS-976 > > > Repository: mesos-git > > > Description > ------- > > internal::Destroyer now uses a single pass of freeze, kill, thaw, reap > to kill all processes in a freezer cgroup. cgroups::destroy will not > complete until all processes have been reaped. A timeout is used for > each step so destroy will eventually fail rather than constantly > retrying. > > Added a test for destroying a freezer cgroup containing a stopped > process. > > > Diffs > ----- > > src/linux/cgroups.hpp 5a5735721fb9f051eee661edb08d1cdaa163d0f3 > src/linux/cgroups.cpp 8202c282f580d027a60ded2081962e96e4860f60 > src/slave/containerizer/isolators/cgroups/cpushare.cpp > b494a9236210245383e20fa9ab3dbac01e42f8dd > src/slave/containerizer/linux_launcher.cpp > 530e0bd64d71bad761a2eab3d6e2f2179a167b4b > src/tests/cgroups_tests.cpp 6ba9de622953e158feadaa9950618b0b13c9e832 > > Diff: https://reviews.apache.org/r/20818/diff/ > > > Testing > ------- > > make check # Linux > > > Thanks, > > Ian Downes > >
