----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36620/#review96872 -----------------------------------------------------------
src/linux/cgroups.cpp (line 1676) <https://reviews.apache.org/r/36620/#comment152560> Hum, what if 'cgroups::processes' fails the first time (i.e., 'chain' hasn't been set up yet)? I would suggest terminating the process if error is encountered: ``` Try<set<pid_t>> processes = ...; if (processes.isError()) { promise.fail("Failed to ...: " + processes.error()); terminate(self()); return; } ``` src/linux/cgroups.cpp (line 1681) <https://reviews.apache.org/r/36620/#comment152561> First of all, no need to pass in 'chain' because it's a member field of this class. Second, I don't think we should call 'finished' here because you can simply do the following and save a call to 'cgroups::procsses' ``` if (processes.get().empty()) { promise.set(Nothing()); terminate(self()); return; } ``` src/linux/cgroups.cpp (lines 1685 - 1686) <https://reviews.apache.org/r/36620/#comment152562> It's not really a chain here. In fact, we don't need a 'chain' variable here. The 'kill' (or 'signal') is a synchronour operation. ALso, no need to pull 'collect(statuses)' into a separate function. Here is my suggestion: ``` statuses.clear(); // Send SIGKILL to all processes. foreach (pid_t pid, processes.get()) { ::kill(...); statuses.push_back(process::reap(pid)); } collect(statuses) .onAny(defer(self(), &Self::_killTasksIteration)); ... void _killTasksIteration(const Future<...>& future) { if (!future.isReady()) { promise.fail(...); terminate(self()); return; } killTasksIteration(); } virtual void finalize() { discard(statuses); promise.discard(); } ``` - Jie Yu On Aug. 28, 2015, 3:48 p.m., Joerg Schad wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/36620/ > ----------------------------------------------------------- > > (Updated Aug. 28, 2015, 3:48 p.m.) > > > Review request for mesos, Benjamin Hindman, Till Toenshoff, and Timothy Chen. > > > Bugs: MESOS-3086 > https://issues.apache.org/jira/browse/MESOS-3086 > > > Repository: mesos > > > Description > ------- > > Added Non-Freezeer Task Killer. > > > Diffs > ----- > > src/linux/cgroups.cpp 6ef42ed1bc719f334d1ac6e90919a1bc1840d31f > > Diff: https://reviews.apache.org/r/36620/diff/ > > > Testing > ------- > > sudo make check > + manual tests > > > Thanks, > > Joerg Schad > >
