----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31276/#review76118 -----------------------------------------------------------
src/tests/memory_test_helper.hpp <https://reviews.apache.org/r/31276/#comment123543> Looks like you are not using those function directly for now. Could you please remove them from the header. Keep the comments in the cpp file above the implementation. src/tests/memory_test_helper.hpp <https://reviews.apache.org/r/31276/#comment123564> Do we still need the async signal safe comments? src/tests/memory_test_helper.hpp <https://reviews.apache.org/r/31276/#comment123547> "The abstraction for controlling the memory usage of a subprocess." src/tests/memory_test_helper.hpp <https://reviews.apache.org/r/31276/#comment123561> "Return the pid of the subprocess". src/tests/memory_test_helper.hpp <https://reviews.apache.org/r/31276/#comment123563> The comments here is not quite clear. Could you please move the comments above `allocateRSSMemory` to here? ``` // Increase the anonymous memory usage (RSS) of the // subprocess. The subprocess will use 'mlock' and // 'memset' to make sure the memory is actually // mapped by the kernel. void increaseRSS(const Bytes& size); ``` src/tests/memory_test_helper.hpp <https://reviews.apache.org/r/31276/#comment123572> Again, let's rename the function and revise the comments to make it more easy to understand: ``` // Increases the page cache used by the subprocess. This // is achieved by writing a temporary file. Try<Nothing> increasePageCache(const Bytes& size); ``` src/tests/memory_test_helper.cpp <https://reviews.apache.org/r/31276/#comment123574> s/allocateRSSMemory/increaseRSS/ Let's remove the 'lock' parameter and assume all page will be locked. We can introduce this parameter later once we have a use case. src/tests/memory_test_helper.cpp <https://reviews.apache.org/r/31276/#comment123579> Please use 'size' in the function arguments and use 'bytes' here: ``` size_t bytes = static_cast<size_t>(size.bytes()); ``` src/tests/memory_test_helper.cpp <https://reviews.apache.org/r/31276/#comment123575> You don't have to use perror or abort here anymore right? So: ``` Try<Nothing> increaseRSS(...) { if (...) { return Error(...); } ... } ``` src/tests/memory_test_helper.cpp <https://reviews.apache.org/r/31276/#comment123573> Looks like this is not used. Please remove it. src/tests/memory_test_helper.cpp <https://reviews.apache.org/r/31276/#comment123580> Ditto. src/tests/memory_test_helper.cpp <https://reviews.apache.org/r/31276/#comment123583> Hum, this makes the page cache increasing depend on the memory allocation. Can we simply construct a 4K buffer on the stack and perform write multiple times? Please do not use any ASSERT or EXPECT in the subprocess. Simply return Error and let the top level function to deal with error conditions. src/tests/memory_test_helper.cpp <https://reviews.apache.org/r/31276/#comment123584> You need a comment explaining why fsync is necessary here. src/tests/memory_test_helper.cpp <https://reviews.apache.org/r/31276/#comment123586> Add a comment about what these constants are. src/tests/memory_test_helper.cpp <https://reviews.apache.org/r/31276/#comment123588> Please do not use EXPECT or ASSERT. simply return non-zero and print error messages in stderr src/tests/memory_test_helper.cpp <https://reviews.apache.org/r/31276/#comment123550> Instead of putting subprocess spawning logic in the constructor, which means you have to use ASSERT inside the constructor. Let's defer the assertion to the actual test. So the interface of MemoryTestHelper will be like: ``` class MemoryTestHelper { public: Try<Nothing> spawn(); Try<pid_t> pid(); Try<Nothing> increaseRSS(const Bytes& size); Try<Nothing> increasePageCache(const Bytes& size); }; ``` src/tests/memory_test_helper.cpp <https://reviews.apache.org/r/31276/#comment123590> Again, let's not put any gtest related stuff in MemoryTestHelper. Please return Error instead. Here and everywhere else. src/tests/memory_test_helper.cpp <https://reviews.apache.org/r/31276/#comment123596> This is a mix of stop and reap. I would suggest we rename it to 'stop' which does the kill and wait. Let's simply kill the process in case the process is stuck. Again, remove all ASSERT and EXPECT. src/tests/memory_test_helper_child.hpp <https://reviews.apache.org/r/31276/#comment123544> s/MemoryTestHelperChild/MemoryTestHelperMain/ src/tests/memory_test_helper_child.hpp <https://reviews.apache.org/r/31276/#comment123541> Do you really need this extra file? Can you move this subcommand definition to memory_test_helper.hpp? src/tests/memory_test_helper_child.hpp <https://reviews.apache.org/r/31276/#comment123546> Why do you need these two fields? Can they be local variable in 'execute()'? - Jie Yu On March 10, 2015, 7:26 p.m., Chi Zhang wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/31276/ > ----------------------------------------------------------- > > (Updated March 10, 2015, 7:26 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 > ------- > > Added cgroup memory pressure listening tests. > > > Diffs > ----- > > src/Makefile.am 3059818231c46484039d179cd6916932eff6cd68 > src/tests/cgroups_tests.cpp 75c61aad80f894acb92a9752e8d1b6af70e5b9a6 > src/tests/memory_test_helper.hpp PRE-CREATION > src/tests/memory_test_helper.cpp PRE-CREATION > src/tests/memory_test_helper_child.hpp PRE-CREATION > src/tests/memory_test_helper_main.cpp PRE-CREATION > > Diff: https://reviews.apache.org/r/31276/diff/ > > > Testing > ------- > > > Thanks, > > Chi Zhang > >
