----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30545/#review72242 -----------------------------------------------------------
src/linux/cgroups.hpp <https://reviews.apache.org/r/30545/#comment118269> if the caller is responsible for the lifetime of the process, this should be a unique_ptr (or Owned) src/linux/cgroups.hpp <https://reviews.apache.org/r/30545/#comment118270> s/funtiion/function/ src/linux/cgroups.hpp <https://reviews.apache.org/r/30545/#comment118271> i don't think you need to specify what 0 means. src/linux/cgroups.cpp <https://reviews.apache.org/r/30545/#comment118272> = on the previous line src/linux/cgroups.cpp <https://reviews.apache.org/r/30545/#comment118273> i'm confused at the difference between listen and counter. src/linux/cgroups.cpp <https://reviews.apache.org/r/30545/#comment118274> i don't understand why the first reading is special in this case. src/linux/cgroups.cpp <https://reviews.apache.org/r/30545/#comment118275> you also create an EventListenerProcess in listen above. is that expected? I would have thought just one process would be created. src/linux/cgroups.cpp <https://reviews.apache.org/r/30545/#comment118276> this is very similar to EventListenerProcess::listen. i have the sense this API and implementation can be simplified quite a bit. src/tests/cgroups_tests.cpp <https://reviews.apache.org/r/30545/#comment118277> looks like you can use the other helper method here and make this a one-liner. return stat().get(metric).get(); src/tests/cgroups_tests.cpp <https://reviews.apache.org/r/30545/#comment118279> ASSERT_NE(MAP_FAILED, mapped) src/tests/cgroups_tests.cpp <https://reviews.apache.org/r/30545/#comment118278> EXPECT_EQ so it tears down on failure. src/tests/cgroups_tests.cpp <https://reviews.apache.org/r/30545/#comment118282> helper method to create a listener at a given level? src/tests/cgroups_tests.cpp <https://reviews.apache.org/r/30545/#comment118283> move the three AWAIT_READY calls to after the criticalCounter has been assigned so that we don't (potentially) wait for 10 seconds for each one. src/tests/cgroups_tests.cpp <https://reviews.apache.org/r/30545/#comment118285> EXPECT_GE(lowCounter.get(), 0) reads much more naturally, doesn't it? src/tests/cgroups_tests.cpp <https://reviews.apache.org/r/30545/#comment118284> you should have a maximum time or loop counter here to avoid this potentially running for a long time. src/tests/cgroups_tests.cpp <https://reviews.apache.org/r/30545/#comment118286> can you not put explicit expectations on these event counters? src/tests/memory_test_helper.cpp <https://reviews.apache.org/r/30545/#comment118287> this will fail if rss is NULL but you have the callsites checking for NULL rss. ie, you don't need to check for NULL rss at the callsites. src/tests/memory_test_helper.cpp <https://reviews.apache.org/r/30545/#comment118288> const char ...NAME[] = "..."; src/tests/memory_test_helper.cpp <https://reviews.apache.org/r/30545/#comment118289> const char[] src/tests/memory_test_helper.cpp <https://reviews.apache.org/r/30545/#comment118290> const char[] - Dominic Hamon On Feb. 12, 2015, 12:08 p.m., Chi Zhang wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/30545/ > ----------------------------------------------------------- > > (Updated Feb. 12, 2015, 12:08 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 39ce858fd009f8aef388bd3d6b17aa4c885ff9c9 > 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 > >
