----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31012/#review77678 -----------------------------------------------------------
src/tests/cgroups_tests.cpp <https://reviews.apache.org/r/31012/#comment125852> It's unnecessarily confusing to have a public and private stat() plust stat_() and _stat ! ;-) How about statistic(const std::string) and hashmap statistics() _stat() is really taking a sample()? src/tests/cgroups_tests.cpp <https://reviews.apache.org/r/31012/#comment125851> statistics()? src/tests/cgroups_tests.cpp <https://reviews.apache.org/r/31012/#comment125854> _stat() only has EXPECT_SOME(), therefore stat_ is not necessarily isSome(), and .get() will abort. src/tests/cgroups_tests.cpp <https://reviews.apache.org/r/31012/#comment125857> Can you write a brief description of the test purpose, for each of them src/tests/cgroups_tests.cpp <https://reviews.apache.org/r/31012/#comment125861> where is allocateRSSMemory and deallocateRSSMemory, I can't see them in this review or r30545 src/tests/cgroups_tests.cpp <https://reviews.apache.org/r/31012/#comment125858> (default - delta) < stat("rss") < (default + delta) is equivalent, and much more clearly expressed as: |stat - default| < delta src/tests/cgroups_tests.cpp <https://reviews.apache.org/r/31012/#comment125859> why do you need to deallocate? shouldn't everything get cleaned up when the test completes? I assume that each test is independent...? src/tests/cgroups_tests.cpp <https://reviews.apache.org/r/31012/#comment125860> ditto src/tests/cgroups_tests.cpp <https://reviews.apache.org/r/31012/#comment125862> drop the 1 suffix, call it something that makes it clearer that it's a reading before allocations. src/tests/cgroups_tests.cpp <https://reviews.apache.org/r/31012/#comment125863> +1 the test should be run in a temporary directory that always gets cleaned up in tear down, see TemporaryDirectoryTest src/tests/cgroups_tests.cpp <https://reviews.apache.org/r/31012/#comment125855> NULL pointer is converted to false, so ASSERT_TRUE(rss) should work src/tests/cgroups_tests.cpp <https://reviews.apache.org/r/31012/#comment125864> path doesn't get cleaned up. src/tests/cgroups_tests.cpp <https://reviews.apache.org/r/31012/#comment125865> generally it's cleaner, i.e., avoids extra indentation to structure as: if (pid == 0) { // do whatever in the child // exit or loop } // continue in parent src/tests/cgroups_tests.cpp <https://reviews.apache.org/r/31012/#comment125867> if you assert here then the child is not killed? will it be cleaned up at test tear down? src/tests/cgroups_tests.cpp <https://reviews.apache.org/r/31012/#comment125866> why keep trying here but not in the parent...? src/tests/cgroups_tests.cpp <https://reviews.apache.org/r/31012/#comment125868> ditto src/tests/cgroups_tests.cpp <https://reviews.apache.org/r/31012/#comment125869> ditto - Ian Downes On Feb. 13, 2015, 10:08 a.m., Chi Zhang wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/31012/ > ----------------------------------------------------------- > > (Updated Feb. 13, 2015, 10:08 a.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 tests for memory statistics. > > > Diffs > ----- > > src/tests/cgroups_tests.cpp 9d50a47c939f5f136c85fdcc555459512c6f2259 > > Diff: https://reviews.apache.org/r/31012/diff/ > > > Testing > ------- > > > Thanks, > > Chi Zhang > >
