----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/9094/#review16276 -----------------------------------------------------------
include/mesos/mesos.proto <https://reviews.apache.org/r/9094/#comment34743> ... since the Epoch. include/mesos/mesos.proto <https://reviews.apache.org/r/9094/#comment34744> Are these total times or since the last snapshot? If so, what's the interval or do I need to have a previous ResourceUsage to see it's timestamp? src/files/files.cpp <https://reviews.apache.org/r/9094/#comment34745> Yeah, I don't think we want this to propagate. src/slave/isolation_module.hpp <https://reviews.apache.org/r/9094/#comment34748> Just curious, what did you need this for? Maybe a comment? src/slave/isolation_module.hpp <https://reviews.apache.org/r/9094/#comment34749> Formatting. src/slave/monitor.hpp <https://reviews.apache.org/r/9094/#comment34753> const& src/slave/monitor.cpp <https://reviews.apache.org/r/9094/#comment34759> const& src/slave/monitor.cpp <https://reviews.apache.org/r/9094/#comment34760> Yeah const&! ;) src/slave/monitor.cpp <https://reviews.apache.org/r/9094/#comment34763> I like the notion of the 'expose' verb, but I feel like I read it better 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 implementation doesn't look like it needs to be a method but could just be a function. src/slave/monitor.cpp <https://reviews.apache.org/r/9094/#comment34755> Maybe it should propagate until we fix it? src/slave/monitor.cpp <https://reviews.apache.org/r/9094/#comment34762> So I get that you don't need this to be a method of ResourceMonitorProcess, 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 ResourceMonitorProcess::usage. src/slave/monitor.cpp <https://reviews.apache.org/r/9094/#comment34761> A comment on why you're not using const& here would be nice. src/slave/monitor.cpp <https://reviews.apache.org/r/9094/#comment34764> 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 just an 'rss' field? src/slave/monitor.cpp <https://reviews.apache.org/r/9094/#comment34757> 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 <https://reviews.apache.org/r/9094/#comment34758> Ditto, lambda::_1 early and now std::tr1::placeholders::_1? src/slave/slave.cpp <https://reviews.apache.org/r/9094/#comment34747> 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 referenced by flags in constants.hpp/cpp since it might be a source of confusion. third_party/libprocess/src/statistics.cpp <https://reviews.apache.org/r/9094/#comment34766> How does the added output help us debug? IIUC, if one of these CHECKs fires there is a serious bug, regardless of the context and name. Am I missing 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 module > 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 reviews. > > > 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 > 669efa14ba2603764aa68ae19a44e79dbfdec192 > src/slave/cgroups_isolation_module.cpp > 63cefc33cf34eebb82db5d8448b751be8652fa36 > src/slave/constants.hpp ddf02570caf3793106b3c48e158a5bb48c1ae80c > src/slave/constants.cpp 1735a6b55a93e6537a5a119e5345961f3d84a000 > src/slave/isolation_module.hpp b962365ebeddd047896a66b02a327aa26ae323d3 > src/slave/lxc_isolation_module.hpp 2bc844f491befbe588965da2ada7cfcef0b6f0a4 > src/slave/lxc_isolation_module.cpp 30cff2a49339bb07030727d30352536a0a22d58c > src/slave/monitor.hpp PRE-CREATION > src/slave/monitor.cpp PRE-CREATION > src/slave/process_based_isolation_module.hpp > f1817192582e3646f8dcf17934ba7998829e8fd6 > src/slave/process_based_isolation_module.cpp > 3d50a4b652e4e09dd57e744e408c8fb79ff3fbf5 > src/slave/slave.hpp e9f7b659ca2860501840b3d01e69915ebd162039 > src/slave/slave.cpp 9755b46f97173d6fcc9ab1fd63e0e4814b3bc018 > src/tests/utils.hpp be457117515ee727af101370b26bf9188afb8f45 > third_party/libprocess/include/process/statistics.hpp > 9e3041a6e2a8ef022eacacad00bc4d974a8e33c9 > third_party/libprocess/src/statistics.cpp > 2fe8af83c6c63a0fa8cb2e9636f9289f0e3d7f2f > third_party/libprocess/src/tests/statistics_tests.cpp > 0aaab3526618171c7cfbd11d40d614344bcbfd0a > > 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 future > reviews. > > > Thanks, > > Ben Mahler > >
