Return-Path: 
 <reviews-return-8339-apmail-mesos-reviews-archive=mesos.apache.org@mesos.apache.org>
X-Original-To: apmail-mesos-reviews-archive@minotaur.apache.org
Delivered-To: apmail-mesos-reviews-archive@minotaur.apache.org
Received: from mail.apache.org (hermes.apache.org [140.211.11.3])
	by minotaur.apache.org (Postfix) with SMTP id 713F61828A
	for <apmail-mesos-reviews-archive@minotaur.apache.org>;
 Fri, 28 Aug 2015 16:58:34 +0000 (UTC)
Received: (qmail 99952 invoked by uid 500); 28 Aug 2015 16:58:34 -0000
Delivered-To: apmail-mesos-reviews-archive@mesos.apache.org
Received: (qmail 99930 invoked by uid 500); 28 Aug 2015 16:58:34 -0000
Mailing-List: contact reviews-help@mesos.apache.org; run by ezmlm
Precedence: bulk
List-Help: <mailto:reviews-help@mesos.apache.org>
List-Unsubscribe: <mailto:reviews-unsubscribe@mesos.apache.org>
List-Post: <mailto:reviews@mesos.apache.org>
List-Id: <reviews.mesos.apache.org>
Reply-To: reviews@mesos.apache.org
Delivered-To: mailing list reviews@mesos.apache.org
Received: (qmail 99905 invoked by uid 99); 28 Aug 2015 16:58:34 -0000
Received: from reviews-vm.apache.org (HELO reviews.apache.org) (140.211.11.40)
    by apache.org (qpsmtpd/0.29) with ESMTP; Fri, 28 Aug 2015 16:58:34 +0000
Received: from reviews.apache.org (localhost [127.0.0.1])
	by reviews.apache.org (Postfix) with ESMTP id 2584E26CB48;
	Fri, 28 Aug 2015 16:58:33 +0000 (UTC)
Content-Type: multipart/alternative;
 boundary="===============6720030318583752997=="
MIME-Version: 1.0
Subject: Re: Review Request 36620: Added Non-Freezeer Task Killer.
From: "Joerg Schad" <joerg@mesosphere.io>
To: "Benjamin Hindman" <benh@berkeley.edu>,
 "Timothy Chen" <tnachen@apache.org>,
 "Till Toenshoff" <toenshoff@me.com>
Cc: "mesos" <reviews@mesos.apache.org>, "Jie Yu" <yujie.jay@gmail.com>,
 "Joerg Schad" <joerg@mesosphere.io>
Date: Fri, 28 Aug 2015 16:58:33 -0000
Message-ID: <20150828165833.13583.82765@reviews.apache.org>
X-ReviewBoard-URL: https://reviews.apache.org/
Auto-Submitted: auto-generated
Sender: "Joerg Schad" <noreply@reviews.apache.org>
X-ReviewGroup: mesos
X-Auto-Response-Suppress: DR, RN, OOF, AutoReply
X-ReviewRequest-URL: https://reviews.apache.org/r/36620/
X-Sender: "Joerg Schad" <noreply@reviews.apache.org>
References: <20150827190243.13584.63375@reviews.apache.org>
In-Reply-To: <20150827190243.13584.63375@reviews.apache.org>
Reply-To: "Joerg Schad" <joerg@mesosphere.io>
X-ReviewRequest-Repository: mesos

--===============6720030318583752997==
MIME-Version: 1.0
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: 7bit



> On Aug. 27, 2015, 7:02 p.m., Jie Yu wrote:
> > src/linux/cgroups.cpp, lines 1776-1791
> > <https://reviews.apache.org/r/36620/diff/13/?file=1039336#file1039336line1776>
> >
> >     Could you please introduce a new function under cgroups namespace and put this logic there:
> >     
> >     ```
> >     // Kill all processes in the given cgroup.
> >     Future<Nothing> cgroups::kill(
> >         const string& hierarchy,
> >         const string& cgroup)
> >     {
> >       ...
> >       if (freezerCheckError.isNone()) {
> >       } else {
> >       }
> >       
> >       return ...;
> >     }
> >     ```
> >     
> >     You may want to rename the exsiting `cgroups::kill(hierarchy, cgroup, signal)` to `cgroups::signal`.
> 
> Timothy Chen wrote:
>     Hi jie, thanks for chiming in. This should be ready after the comments are addressed. I'll try to merge this as well when it's done. I'll ping Joerg about this.
> 
> Joerg Schad wrote:
>     Hi Jie,
>     I am not sure whether I should move the entire logic further into cgroups::kill. Both path (freezer vs Non-freezer) have distinct logic on how to destroy the cgroups, which in my understanding makes sense to have in seperate TasksKiller entities. Secondly cgroups::kill currently is little more than trying to kill the cgroup, when moving the logic in here cgroups::kill would have to call cgroups::freeze or call the iterative logic. Secondly, in my understanding the interface here is the destroy() call. I actually would like to refactor this piece of code in general, but this is outside the scope of this patch. If we want to discuss this further -or misunderstand something here- I would be happy to discuss it further!
> 
> Jie Yu wrote:
>     Hi Joerg,
>     
>     I think you misunderstood what I propose here. I am not asking you to merge the logics of TasksKiller and FreezerTasksKiller. I think it's great that you separate these two Processes. What I am sugguesting here is that you pull the code from 1777-1790 to a separate function (like the following). We may want to reuse that function in Mesos containerizer launcher in the future (i.e., not rely on freezer). It also makes the logics in Destroyer cleaner. Let me know your thoughts! Thank you for taking on this!
>     
>     ```
>     virtual void initialize()
>     {
>       // Stop when no one cares.
>       promise.future().onDiscard(lambda::bind(
>           static_cast<void (*)(const UPID&, bool)>(terminate), self(), true));
>     
>       // Kill tasks in the given cgroups in parallel. Use collect mechanism to
>       // wait until all kill processes finish.
>       foreach (const string& cgroup, cgroups) {
>         killers.push_back(cgroups:kill(hierarchy, cgroup));
>       }
>     
>       collect(killers)
>         .onAny(defer(self(), &Destroyer::killed, lambda::_1));
>     }
>     
>     
>     // Kill all processes in the given cgroup.
>     Future<Nothing> cgroups::kill(
>         const string& hierarchy,
>         const string& cgroup)
>     {
>       Future<Nothing> future;
>       
>       // Use the freezer subsystem if available.
>       Option<Error> freezerCheckError =
>         verify(hierarchy, cgroup, "freezer.state");
>       
>       if (freezerCheckError.isNone()) {
>         internal::FreezerTasksKiller* killer =
>           new internal::FreezerTasksKiller(hierarchy, cgroup);
>          
>         future = killer->future();
>     
>         spawn(killer, true);
>       } else {
>         internal::TasksKiller* killer =
>           new internal::TasksKiller(hierarchy, cgroup);
>           
>         future = killer->future();
>         
>         spawn(killer, true);
>       }
>       
>       return future;
>     }
>     ```

As the renaming seems to be controversial (https://reviews.apache.org/r/37894/), I would right now introduce cgroups::killProcesses. Does that work for you?


- Joerg


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36620/#review96734
-----------------------------------------------------------


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


--===============6720030318583752997==--