----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30545/#review70772 -----------------------------------------------------------
src/linux/cgroups.hpp <https://reviews.apache.org/r/30545/#comment116179> does this mean that the callback is called if the pressure is >= to the callback pressure? ie, if i have a low and medium callback installed and the memory pressure is critical, both my callbacks will be called? src/linux/cgroups.cpp <https://reviews.apache.org/r/30545/#comment116180> why are you passing through an argument and then not using it in the continuation? src/linux/cgroups.cpp <https://reviews.apache.org/r/30545/#comment116183> i wish this was a unique_ptr. can we please get this cleared as a good use case for unique_ptr? src/linux/cgroups.cpp <https://reviews.apache.org/r/30545/#comment116184> "memory.pressure_level" should be a constant in the class i think. just in case we find it's in a different place in different OSes. src/linux/cgroups.cpp <https://reviews.apache.org/r/30545/#comment116182> we don't catch this anywhere else in the code. kill the check. src/linux/cgroups.cpp <https://reviews.apache.org/r/30545/#comment116185> you don't store or delete the listeners themselves. that's a memory leak. src/linux/cgroups.cpp <https://reviews.apache.org/r/30545/#comment116187> when is this deleted? src/linux/cgroups.cpp <https://reviews.apache.org/r/30545/#comment116186> no need for this check. src/tests/cgroups_tests.cpp <https://reviews.apache.org/r/30545/#comment116189> if you have ASSERT_EQ in the child process, then you should be able to use ASSERT_SOME() for assign above and shouldn't need the cerr logging or abort calls. src/tests/cgroups_tests.cpp <https://reviews.apache.org/r/30545/#comment116190> this seems like it shouldbe the start of another test. maybe you want a test fixture for memory pressure that sets up the above, and then a series of smaller tests: one tests the discard, and one tests normal operation. also, you should be testing low/medium/critical callbacks separately and making sure the expectations of how often each is hit are satisfied. src/tests/cgroups_tests.cpp <https://reviews.apache.org/r/30545/#comment116191> we already have timers to do this. don't reinvent the wheel. src/tests/cgroups_tests.cpp <https://reviews.apache.org/r/30545/#comment116192> on oom? you mean a critical memory pressure event? - Dominic Hamon On Feb. 2, 2015, 9:28 p.m., Chi Zhang wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/30545/ > ----------------------------------------------------------- > > (Updated Feb. 2, 2015, 9:28 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/linux/cgroups.hpp abf31df1b4dbf6f715f93256b83c9996a45099cf > src/linux/cgroups.cpp 0b136e17907123e814e6011523d8e66a4d66cf98 > src/tests/cgroups_tests.cpp 1eea57f217cd51a6cf0a59ca80823251333563c6 > > Diff: https://reviews.apache.org/r/30545/diff/ > > > Testing > ------- > > > Thanks, > > Chi Zhang > >
