Return-Path: X-Original-To: apmail-incubator-mesos-dev-archive@minotaur.apache.org Delivered-To: apmail-incubator-mesos-dev-archive@minotaur.apache.org Received: from mail.apache.org (hermes.apache.org [140.211.11.3]) by minotaur.apache.org (Postfix) with SMTP id 0964DE84A for ; Mon, 26 Nov 2012 18:46:09 +0000 (UTC) Received: (qmail 22396 invoked by uid 500); 26 Nov 2012 18:46:08 -0000 Delivered-To: apmail-incubator-mesos-dev-archive@incubator.apache.org Received: (qmail 22359 invoked by uid 500); 26 Nov 2012 18:46:08 -0000 Mailing-List: contact mesos-dev-help@incubator.apache.org; run by ezmlm Precedence: bulk List-Help: List-Unsubscribe: List-Post: List-Id: Reply-To: mesos-dev@incubator.apache.org Delivered-To: mailing list mesos-dev@incubator.apache.org Received: (qmail 22347 invoked by uid 99); 26 Nov 2012 18:46:08 -0000 Received: from reviews-vm.apache.org (HELO reviews.apache.org) (140.211.11.40) by apache.org (qpsmtpd/0.29) with ESMTP; Mon, 26 Nov 2012 18:46:08 +0000 Received: from reviews.apache.org (localhost [127.0.0.1]) by reviews.apache.org (Postfix) with ESMTP id E34001C4CA2; Mon, 26 Nov 2012 18:46:04 +0000 (UTC) Content-Type: multipart/alternative; boundary="===============7239066165950876258==" MIME-Version: 1.0 Subject: Re: Review Request: Replaced some names in cgroups and updated some return values to be Try instead of Try. From: "Ben Mahler" To: "Vinod Kone" , "Ben Mahler" Cc: "mesos" , "Benjamin Hindman" Date: Mon, 26 Nov 2012 18:46:04 -0000 Message-ID: <20121126184604.26366.24263@reviews.apache.org> X-ReviewBoard-URL: https://reviews.apache.org Auto-Submitted: auto-generated Sender: "Ben Mahler" X-ReviewGroup: mesos X-ReviewRequest-URL: https://reviews.apache.org/r/8058/ X-Sender: "Ben Mahler" References: <20121115221100.10558.68064@reviews.apache.org> In-Reply-To: <20121115221100.10558.68064@reviews.apache.org> Reply-To: "Ben Mahler" --===============7239066165950876258== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: quoted-printable > On Nov. 15, 2012, 10:11 p.m., Ben Mahler wrote: > > src/linux/cgroups.hpp, line 193 > > > > > > So this one is called 'kill' but it's for sending arbitrary signals= to all processes in a cgroup? Do we enforce the signal is of type SIGKILL,= SIGQUIT, etc.? > = > Benjamin Hindman wrote: > This is called 'kill' to provide the syscall 'kill' functionality for= cgroups (which already exists for processes and process groups). Since thi= s is for sending an arbitrary signal, it does not enforce only SIGKILL or S= IGQUIT. > = > This primitive is useful for implementing a few of our primitives suc= h as 'destroy', but it also seemed generally useful, and thus worth providi= ng. > = > Note that I eliminated 'killTasks' because it shares a lot in common = with 'destroy' (and I felt having two very similar things would be more con= fusing). Instead, I factored out the signal sending part of 'killTasks' int= o 'kill'. My thought was that we can "extend" destroy in the future to perf= orm non-recursive destroys if we want to get some of the 'killTasks' functi= onality back (which isn't needed outside of the cgroups.cpp). I missed that this matches the kill syscall, sounds good. > On Nov. 15, 2012, 10:11 p.m., Ben Mahler wrote: > > src/linux/cgroups.cpp, line 626 > > > > > > Hm.. this is kind of a backwards way to do error handling compared = to how we do it everywhere else. > > = > > Why the switch from Try to Option? > = > Benjamin Hindman wrote: > First, this is different than 'checkHierarchy' which has been replace= d with 'mounted'. > = > Second, this is an internal function that is being used to verify a h= ierarchy (possibly a cgroup, and possibly a control) and return a formatted= error string if an error has occurred. (I suppose the comment above 'verif= y' was insufficient, but this function is effectively called "verifyOrRetur= nFormattedErrorString".) I choose not to return Try because I want= ed the semantics that the returned error string was "complete", and I would= n't need to add any contextual information at the call site. This is not th= e case (or should not be the case) for other things that return Try<*> (e.g= ., mkdir). In fact, this review includes numerous changes where I'm adding = missing contextual information. > = > Said another way, 99.9% of all cases where we "propagate" an error fr= om a Try<*> we add some contextual information, and when that contextual in= formation is not there I (we) tend try and determine whether or not that is= okay or we need to add something (again, I did this a bunch in this review= ). I didn't want to have to remember why I didn't include contextual inform= ation for a Try returned from 'verify', so instead I choose to mak= e the returned error string more explicit. Alright, I'd prefer to keep the consistency, but I see you put a lot of tho= ught into this. - Ben ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/8058/#review13483 ----------------------------------------------------------- On Nov. 14, 2012, 11:57 p.m., Benjamin Hindman wrote: > = > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/8058/ > ----------------------------------------------------------- > = > (Updated Nov. 14, 2012, 11:57 p.m.) > = > = > Review request for mesos, Vinod Kone and Ben Mahler. > = > = > Description > ------- > = > See summary. > = > = > Diffs > ----- > = > src/linux/cgroups.hpp 1351134dc8b40fb7d7d692c4dd24f0efa24eadb0 = > src/linux/cgroups.cpp 039e2b4121edc419e9f1a48a7c0b99ca77aa44ff = > src/slave/cgroups_isolation_module.cpp 8211618d7729350654e2d17946c5b912= ed9dda6a = > src/tests/assert.hpp 62fde12fa7ddddbbc9e8812f26314af5307a02df = > src/tests/cgroups_tests.cpp 85e81854a6767bad5c19f6411130d154c2870d99 = > = > Diff: https://reviews.apache.org/r/8058/diff/ > = > = > Testing > ------- > = > make check > = > = > Thanks, > = > Benjamin Hindman > = > --===============7239066165950876258==--