----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30545/#review72971 -----------------------------------------------------------
First pass, haven't looked at the tests yet. The main issue is that Listener is not thread safe. You probably need to create a libprocess. src/linux/cgroups.hpp <https://reviews.apache.org/r/30545/#comment119089> We usually put the comments above the forward declaration: ``` // Forward declaration. class Listener; ``` src/linux/cgroups.hpp <https://reviews.apache.org/r/30545/#comment119090> +1 on Dominic's comment. Let's create a enum and simply expose the static method 'create' in pressure::Listener. You can kill the forward declaration as well. ``` enum Level { LOW, MEDIUM, CRITICAL }; Try<Owned<Listener>> pressure::Listener::create( const string& hierarchy, const string& cgroup, Level level); ``` src/linux/cgroups.hpp <https://reviews.apache.org/r/30545/#comment119092> Remove "On a single error...", and replace it with "Please see comments of 'counter' for details." src/linux/cgroups.hpp <https://reviews.apache.org/r/30545/#comment119099> What's the concurrency story here? Looks like this class is not thread safe? Some thread is reading counter/error while other thread can modify counter/error. I guess you still need a process here. src/linux/cgroups.hpp <https://reviews.apache.org/r/30545/#comment119091> "Returns a failure if any error occurs while monitoring the pressure events, and any subsequent calls to 'counter' will return the same failure. In that case, the user should consider create a new Listener." src/linux/cgroups.hpp <https://reviews.apache.org/r/30545/#comment119093> See my comments above, please expose this function src/linux/cgroups.hpp <https://reviews.apache.org/r/30545/#comment119095> s/read/listen/? Also, please use const ref for 'future' ``` const Future<uint64_t>& future ``` src/linux/cgroups.hpp <https://reviews.apache.org/r/30545/#comment119098> Let's not conflate 'counter' and 'error'. Use two member fields: ``` Option<Error> error; uint64_t counter_; (initialize it to 0) ``` src/linux/cgroups.cpp <https://reviews.apache.org/r/30545/#comment119094> Kill those functions. src/linux/cgroups.cpp <https://reviews.apache.org/r/30545/#comment119096> s/read/listen src/linux/cgroups.cpp <https://reviews.apache.org/r/30545/#comment119097> s/read/listen/ - Jie Yu On Feb. 17, 2015, 8:12 p.m., Chi Zhang wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/30545/ > ----------------------------------------------------------- > > (Updated Feb. 17, 2015, 8:12 p.m.) > > > Review request for mesos, Dominic Hamon, Ian Downes, and Jie Yu. > > > Bugs: MESOS-2136 > https://issues.apache.org/jira/browse/MESOS-2136 > > > Repository: mesos > > > Description > ------- > > cgroups: added support to listen on memory pressures. > > > Diffs > ----- > > src/Makefile.am d372404d84c9ac0bc49da7407ad366701b9586a6 > src/linux/cgroups.hpp bf8efee05b995a37d4e6e7679a493b2d5238aa0b > src/linux/cgroups.cpp b6f75b14dea7609b90627eccdd33ef891ed9d21f > src/tests/cgroups_tests.cpp 9d50a47c939f5f136c85fdcc555459512c6f2259 > src/tests/memory_test_helper.hpp PRE-CREATION > src/tests/memory_test_helper.cpp PRE-CREATION > src/tests/memory_test_helper_main.cpp PRE-CREATION > > Diff: https://reviews.apache.org/r/30545/diff/ > > > Testing > ------- > > > Thanks, > > Chi Zhang > >
