----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31276/#review73684 -----------------------------------------------------------
I really like the new memory test helper! I can also help us simply other cgroups tests as well as the ballon framework! The high level comment I have for this patch is that we should hide some details of the memory test helper. For example, right now, we use `EXPECT_SOME(os::write(s.get().in().get(), "A\n"));` in the tests to signal the child to continue. This is really implicit and we should hide it (at least the magical 'A' :)) Here is what I suggest. Create a MemoryTestHelper class. This class will internally spawn a subprocess, save the pid and provide various interfaces to the tests. For example, `helper.increaseRSS(const Bytes& size)` will increase the RSS of the child process. `helper.increasePageCache(const Bytes& size)` will increase the page cache used by the child process. `helper.stop()` will stop the child process. In that way, the caller (i.e., tests) will become less verbose and more explicit! src/Makefile.am <https://reviews.apache.org/r/31276/#comment120072> Any reaon this is only for linux? I would imagine that this will be useful for OSX as well. src/tests/cgroups_tests.cpp <https://reviews.apache.org/r/31276/#comment120073> Please do a rebase please:) src/tests/cgroups_tests.cpp <https://reviews.apache.org/r/31276/#comment120104> Let's do that in a different patch. src/tests/memory_test_helper.cpp <https://reviews.apache.org/r/31276/#comment120105> I am curious if we can merge these two because their logics are pretty similar: 1) wait some command from parent 2) do something 3) repeat 1)-2) unless the command is stop src/tests/memory_test_helper.cpp <https://reviews.apache.org/r/31276/#comment120106> Per our discussion, can we keep track of the temp files you created in the child and delete them on the stop command? In that way, the parent does not need to pass the file path which seems to be a little weired. src/tests/memory_test_helper_main.cpp <https://reviews.apache.org/r/31276/#comment120071> Per our discussion, could you please have a separate patch which move the main function in setns_test_helper.cpp to setns_test_helper_main.cpp? - Jie Yu On Feb. 22, 2015, 5:14 a.m., Chi Zhang wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/31276/ > ----------------------------------------------------------- > > (Updated Feb. 22, 2015, 5:14 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 > ------- > > Added cgroup memory pressure listening tests. > > > Diffs > ----- > > src/Makefile.am d372404d84c9ac0bc49da7407ad366701b9586a6 > 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/31276/diff/ > > > Testing > ------- > > > Thanks, > > Chi Zhang > >
