----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31276/#review77779 -----------------------------------------------------------
Overall looks good. Let's sync on the test body. src/tests/cgroups_tests.cpp <https://reviews.apache.org/r/31276/#comment126074> Can you set the limit in the test? src/tests/cgroups_tests.cpp <https://reviews.apache.org/r/31276/#comment126073> Same as that in the memory isolator, put that into a hashmap? Therefore, you don't need to use 'Option'. src/tests/cgroups_tests.cpp <https://reviews.apache.org/r/31276/#comment126071> I would rather move those fields to each tests. They are temporary variables. src/tests/cgroups_tests.cpp <https://reviews.apache.org/r/31276/#comment126076> This is a little unreliable and non-deterministic. Any reason why you want to call increaseRSS multiple times? Can you increase the RSS once to a size right below 'limit' and keep reading the counter until it shows non-zero (and set a time limit for that). src/tests/cgroups_tests.cpp <https://reviews.apache.org/r/31276/#comment126077> I don't quite get this part :( src/tests/memory_test_helper.hpp <https://reviews.apache.org/r/31276/#comment125990> Please add a comment saying that this is a blocking call. src/tests/memory_test_helper.hpp <https://reviews.apache.org/r/31276/#comment126000> I won't mention the 1MB buffer in the comment. This is impl. detail. Just mention you'll use a buffer and doing multiple writes. Also, don't mention the rounding because that's impl. detail as well. You can add a TODO in the impl. src/tests/memory_test_helper.cpp <https://reviews.apache.org/r/31276/#comment126054> kill this. WE typically use lambda::function directly in the code base. src/tests/memory_test_helper.cpp <https://reviews.apache.org/r/31276/#comment125998> ``` return Error("Expect at least one argument"); ``` src/tests/memory_test_helper.cpp <https://reviews.apache.org/r/31276/#comment125992> ``` return Error("The first argument '" + tokens[1] + "' is not a byte size"); ``` src/tests/memory_test_helper.cpp <https://reviews.apache.org/r/31276/#comment125999> Since allocateRSS is only used here, please kill the helper funciton and move the logic into increaseRSS src/tests/memory_test_helper.cpp <https://reviews.apache.org/r/31276/#comment126044> Move this down after validation is done. Also, s/memory/buffer/ src/tests/memory_test_helper.cpp <https://reviews.apache.org/r/31276/#comment126045> Ditto. Please make it more informative. src/tests/memory_test_helper.cpp <https://reviews.apache.org/r/31276/#comment126048> s/bytes/size/ src/tests/memory_test_helper.cpp <https://reviews.apache.org/r/31276/#comment126046> Ditto. src/tests/memory_test_helper.cpp <https://reviews.apache.org/r/31276/#comment126047> Why this is necessary? src/tests/memory_test_helper.cpp <https://reviews.apache.org/r/31276/#comment126049> NOTE: The child ... src/tests/memory_test_helper.cpp <https://reviews.apache.org/r/31276/#comment126051> Can you simply use this existing interface: ``` inline Try<Nothing> write(int fd, const std::string& message) ``` That means, instead of using a char[] buffer, use a string as the buffer. You can leverage this string constructor: ``` string (size_t n, char c); ``` That means you don't need the helper function as well. Please move the logics down here. src/tests/memory_test_helper.cpp <https://reviews.apache.org/r/31276/#comment126056> No need to create a helper funciton for that. Please just move the hashmap initialization to 'execute()' below. src/tests/memory_test_helper.cpp <https://reviews.apache.org/r/31276/#comment126059> YOu are not checking the result of getline. This is not a good programming practice. Any reason not using: ``` while (getline(cin, line) >= 0) { .... } if (!cin.eof()) { cerr << "" << endl; return 1; } return 0; ``` src/tests/memory_test_helper.cpp <https://reviews.apache.org/r/31276/#comment126060> WHat if the token size is 0? It's good to be defensive. ``` if (tokens.empty()) { cerr << "" << endl; return 1; } ``` src/tests/memory_test_helper.cpp <https://reviews.apache.org/r/31276/#comment126061> Let's add a contains check first: ``` if (!commands.contains(tokens[0])) { cerr << "" << endl; return 1; } Try<Nothing> result = commands[tokens[0]](tokens); ``` src/tests/memory_test_helper.cpp <https://reviews.apache.org/r/31276/#comment126064> s/sizeof(char)/sizeof(STARTED)/ src/tests/memory_test_helper.cpp <https://reviews.apache.org/r/31276/#comment126065> Any reason not using process::reap()? src/tests/memory_test_helper.cpp <https://reviews.apache.org/r/31276/#comment126069> You probably want to inject "\n" here, rather than below. src/tests/memory_test_helper.cpp <https://reviews.apache.org/r/31276/#comment126070> Again, sizeof(DONE) - Jie Yu On March 24, 2015, 10:15 p.m., Chi Zhang wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/31276/ > ----------------------------------------------------------- > > (Updated March 24, 2015, 10:15 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.cpp PRE-CREATION > > Diff: https://reviews.apache.org/r/31276/diff/ > > > Testing > ------- > > > Thanks, > > Chi Zhang > >
