----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31276/#review76978 -----------------------------------------------------------
Much better! src/Makefile.am <https://reviews.apache.org/r/31276/#comment124731> Can you remind me why this is needed? Looks like it's already in mesos_tests_SOURCES? src/tests/memory_test_helper.hpp <https://reviews.apache.org/r/31276/#comment124732> Why do you need to allocate RSS for increase page cache? For example, can you do more writes and use a smaller enough (thus negalegible) buffer on the stack? src/tests/memory_test_helper.cpp <https://reviews.apache.org/r/31276/#comment124736> These are not needed anymore, right. Please do a sweep to remove all headers that are no longer needed. src/tests/memory_test_helper.cpp <https://reviews.apache.org/r/31276/#comment124737> Ditto. src/tests/memory_test_helper.cpp <https://reviews.apache.org/r/31276/#comment124733> As we discussed, can you move the logic in this header to memory_test_helper.hpp? src/tests/memory_test_helper.cpp <https://reviews.apache.org/r/31276/#comment124889> s/RSS/INCREASE_RSS/ src/tests/memory_test_helper.cpp <https://reviews.apache.org/r/31276/#comment124890> s/PAGECACHE/INCREASE_PAGE_CACHE/ src/tests/memory_test_helper.cpp <https://reviews.apache.org/r/31276/#comment124738> do {} while (..) is more suitble here. Also, do you also want to check the failure case (i.e. getline returns -1)? I would suggest using: ``` string line; while(getline(cin, line) >= 0) { // xxx } ``` src/tests/memory_test_helper.cpp <https://reviews.apache.org/r/31276/#comment124876> I would suggest parsing command first and then in each case, parse the argument(s). What if in the future we want to add command without arguments? ``` if (tokens.size() < 1) { cerr << "xxx" << endl; return 1; } string command = tokens[0]; if (command == INCREASE_RSS) { Try<Nothing> execution = increaseRSS(tokens); if (execution.isError()) { cerr << "xxx" << execution.error() << endl; return 1; } } else if (command == INCREASE_PAGE_CACHE) { Try<Nothing> execution = increasePageCache(tokens); if (execution.isError()) { cerr << "xxx" << execution.error() << endl; return 1; } } else { cerr << "xxx" << endl; return 1; } ``` src/tests/memory_test_helper.cpp <https://reviews.apache.org/r/31276/#comment124740> Instead of using abort, can you just return 1 here? Here and everywhere else. src/tests/memory_test_helper.cpp <https://reviews.apache.org/r/31276/#comment124897> s/terminate/cleanup/ Let's make that a private member funciton of MemoryTestHelper so that you can get the subprocess directly without passing in the pid. ``` // NOTE: This function will be blocked until the subprocess is reaped. void MemoryTestHelper::cleanup() { if (s.isSome()) { kill(s.get().pid(), SIGKILL); reap(s.get().pid()).await(); s = None(); } } ``` src/tests/memory_test_helper.cpp <https://reviews.apache.org/r/31276/#comment124898> What if the user calls `spawn` twice? Should you check if `s.isSome()`? src/tests/memory_test_helper.cpp <https://reviews.apache.org/r/31276/#comment124892> The indents for arguments should be 4 spaces. src/tests/memory_test_helper.cpp <https://reviews.apache.org/r/31276/#comment124893> Can you add a TODO in the header saying that this function is blocking and we may want to return a Future and let the top level code decide whether to block wait or not. I would suggest to use ::getline consistently. Currently, you are mixing single char commands and single line commands. src/tests/memory_test_helper.cpp <https://reviews.apache.org/r/31276/#comment124900> Do you want to cleanup the subprocess as well in this case? src/tests/memory_test_helper.cpp <https://reviews.apache.org/r/31276/#comment124902> Ditto. Do you want to cleanup the subprocess as well in this case? - Jie Yu On March 18, 2015, 6:32 p.m., Chi Zhang wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/31276/ > ----------------------------------------------------------- > > (Updated March 18, 2015, 6:32 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 7a06c7028eca8164b1f5fdea6a7ecd37ee6826bb > 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_main.hpp PRE-CREATION > src/tests/memory_test_helper_main.cpp PRE-CREATION > > Diff: https://reviews.apache.org/r/31276/diff/ > > > Testing > ------- > > > Thanks, > > Chi Zhang > >
