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 2BA9CE3DC for ; Thu, 7 Feb 2013 22:39:54 +0000 (UTC) Received: (qmail 63099 invoked by uid 500); 7 Feb 2013 22:39:54 -0000 Delivered-To: apmail-incubator-mesos-dev-archive@incubator.apache.org Received: (qmail 63075 invoked by uid 500); 7 Feb 2013 22:39:54 -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 63057 invoked by uid 99); 7 Feb 2013 22:39:53 -0000 Received: from reviews-vm.apache.org (HELO reviews.apache.org) (140.211.11.40) by apache.org (qpsmtpd/0.29) with ESMTP; Thu, 07 Feb 2013 22:39:53 +0000 Received: from reviews.apache.org (localhost [127.0.0.1]) by reviews.apache.org (Postfix) with ESMTP id BA7A61C724E; Thu, 7 Feb 2013 22:39:46 +0000 (UTC) Content-Type: multipart/alternative; boundary="===============6493847247673969737==" MIME-Version: 1.0 Subject: Re: Review Request: Resource Monitoring 6: Added ResourceMonitor and updated the isolation API. From: "Benjamin Hindman" To: "Benjamin Hindman" , "Vinod Kone" Cc: "Ben Mahler" , "mesos" Date: Thu, 07 Feb 2013 22:39:46 -0000 Message-ID: <20130207223946.13218.92602@reviews.apache.org> X-ReviewBoard-URL: https://reviews.apache.org Auto-Submitted: auto-generated Sender: "Benjamin Hindman" X-ReviewGroup: mesos X-ReviewRequest-URL: https://reviews.apache.org/r/9094/ X-Sender: "Benjamin Hindman" References: <20130201011259.24984.2809@reviews.apache.org> In-Reply-To: <20130201011259.24984.2809@reviews.apache.org> Reply-To: "Benjamin Hindman" --===============6493847247673969737== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: quoted-printable ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/9094/#review16276 ----------------------------------------------------------- include/mesos/mesos.proto ... since the Epoch. include/mesos/mesos.proto Are these total times or since the last snapshot? If so, what's the int= erval or do I need to have a previous ResourceUsage to see it's timestamp? src/files/files.cpp Yeah, I don't think we want this to propagate. src/slave/isolation_module.hpp Just curious, what did you need this for? Maybe a comment? src/slave/isolation_module.hpp Formatting. src/slave/monitor.hpp const& src/slave/monitor.cpp const& src/slave/monitor.cpp Yeah const&! ;) src/slave/monitor.cpp I like the notion of the 'expose' verb, but I feel like I read it bette= r as 'export'. The former makes it seem like I'm trying to show something, = the latter makes it seem like I'm trying to ship it somewhere (as in, ship = it over to statistics). Also, this is kind of like a macro, and the impleme= ntation doesn't look like it needs to be a method but could just be a funct= ion. src/slave/monitor.cpp Maybe it should propagate until we fix it? src/slave/monitor.cpp So I get that you don't need this to be a method of ResourceMonitorProc= ess, but I've tended to like putting the continuation function immediately = after the other function for readability reasons. That might mean having a = declaration at the top of the file in order to reference it in ResourceMoni= torProcess::usage. src/slave/monitor.cpp A comment on why you're not using const& here would be nice. src/slave/monitor.cpp I wonder if it makes sense to have the names in ResourceUsage map here?= Perhaps we change the protobuf to have a 'memory_rss' field rather than ju= st an 'rss' field? src/slave/monitor.cpp s/buildJson/_usage/ = Given our style of the prefixed '_' for continuations, this reads more = clearly below that this is the continuation. = Also, you used lambda earlier, and now std::tr1? src/slave/monitor.cpp Ditto, lambda::_1 early and now std::tr1::placeholders::_1? src/slave/slave.cpp This needs to be a flag (you can keep the constant if you like, but we = should really only be referencing 'flag.resource_collection_interval'). In = the long term we probably don't need to put constants that are only being r= eferenced by flags in constants.hpp/cpp since it might be a source of confu= sion. third_party/libprocess/src/statistics.cpp How does the added output help us debug? IIUC, if one of these CHECKs f= ires there is a serious bug, regardless of the context and name. Am I missi= ng something? - Benjamin Hindman On Feb. 1, 2013, 1:12 a.m., Ben Mahler wrote: > = > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/9094/ > ----------------------------------------------------------- > = > (Updated Feb. 1, 2013, 1:12 a.m.) > = > = > Review request for mesos, Benjamin Hindman and Vinod Kone. > = > = > Description > ------- > = > This is the meat of the resource monitoring design. > = > -Added a ResourceUsage protobuf. > -Added a ResourceMonitor process that periodically hits the isolation mod= ule for usage information. > -Usage is exported to the Statistics process. > -Usage is available via a JSON endpoint. > = > Implementation of the isolation module code will follow in subsequent rev= iews. > = > = > This addresses bug MESOS-324. > https://issues.apache.org/jira/browse/MESOS-324 > = > = > Diffs > ----- > = > include/mesos/executor.hpp 0ea70528a74bb9ba7d2aaac85d2ff85928363869 = > include/mesos/mesos.proto 38235157d45bdccb676e5c3241c21b585a6f8801 = > src/Makefile.am c94736df660a25b58dc47c07d9c56c3c26152a66 = > src/files/files.cpp 60b567eb62e84ccc99b0b1978733a0ea96560813 = > src/slave/cgroups_isolation_module.hpp 669efa14ba2603764aa68ae19a44e79d= bfdec192 = > src/slave/cgroups_isolation_module.cpp 63cefc33cf34eebb82db5d8448b751be= 8652fa36 = > src/slave/constants.hpp ddf02570caf3793106b3c48e158a5bb48c1ae80c = > src/slave/constants.cpp 1735a6b55a93e6537a5a119e5345961f3d84a000 = > src/slave/isolation_module.hpp b962365ebeddd047896a66b02a327aa26ae323d3 = > src/slave/lxc_isolation_module.hpp 2bc844f491befbe588965da2ada7cfcef0b6= f0a4 = > src/slave/lxc_isolation_module.cpp 30cff2a49339bb07030727d30352536a0a22= d58c = > src/slave/monitor.hpp PRE-CREATION = > src/slave/monitor.cpp PRE-CREATION = > src/slave/process_based_isolation_module.hpp f1817192582e3646f8dcf17934= ba7998829e8fd6 = > src/slave/process_based_isolation_module.cpp 3d50a4b652e4e09dd57e744e40= 8c8fb79ff3fbf5 = > src/slave/slave.hpp e9f7b659ca2860501840b3d01e69915ebd162039 = > src/slave/slave.cpp 9755b46f97173d6fcc9ab1fd63e0e4814b3bc018 = > src/tests/utils.hpp be457117515ee727af101370b26bf9188afb8f45 = > third_party/libprocess/include/process/statistics.hpp 9e3041a6e2a8ef022= eacacad00bc4d974a8e33c9 = > third_party/libprocess/src/statistics.cpp 2fe8af83c6c63a0fa8cb2e9636f92= 89f0e3d7f2f = > third_party/libprocess/src/tests/statistics_tests.cpp 0aaab3526618171c7= cfbd11d40d614344bcbfd0a = > = > Diff: https://reviews.apache.org/r/9094/diff/ > = > = > Testing > ------- > = > make check > = > Although I didn't add tests, I've manually tested end-to-end with my futu= re reviews. > = > = > Thanks, > = > Ben Mahler > = > --===============6493847247673969737==--