----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/51215/#review146331 -----------------------------------------------------------
This patch seems to depend on r/51195, please adjust the dependencies. src/tests/CMakeLists.txt (line 26) <https://reviews.apache.org/r/51215/#comment212705> Please indent like line above. src/tests/containerizer/CMakeLists.txt (lines 26 - 56) <https://reviews.apache.org/r/51215/#comment212718> Are these lines still required? If not, please remove. src/tests/containerizer/memory_test_helper.hpp (line 40) <https://reviews.apache.org/r/51215/#comment212719> Even though this is already `virtual` due to inheritance from `Subcommand`, please do still declare it as virtual ~MemoryTestHelper(); here anyway to document somehow that we override like for other functions here (we do not use `override`). src/tests/containerizer/memory_test_helper.hpp (lines 66 - 67) <https://reviews.apache.org/r/51215/#comment212720> The comment here seems redundant as this is a function we override from base. src/tests/containerizer/memory_test_helper.cpp (line 109) <https://reviews.apache.org/r/51215/#comment212721> This code was just moved, but since we touch it we should update the style a bit, e.g., use os::read(s->out().get(), sizeof(STARTED)); instead of os::read(s.get().out().get(), sizeof(STARTED)); Similar for all the other stout/libprocess wrappers used in this patch. src/tests/containerizer/memory_test_helper.cpp (lines 142 - 143) <https://reviews.apache.org/r/51215/#comment212722> I would have expected this documentation at the declaration even though this is a `private` function. src/tests/containerizer/memory_test_helper.cpp (line 214) <https://reviews.apache.org/r/51215/#comment212723> While we do not use this pattern extensively, when seeing `doX()` I personally anticipate some application of the template pattern, e.g., to implement functionality from some specific `x()` (we use this e.g., in the zookeeeper code where we have e.g., a `names()` calling `doNames()`). Since there is no template pattern here, I think just going with the existing names is less confusing. src/tests/containerizer/memory_test_helper.cpp (line 234) <https://reviews.apache.org/r/51215/#comment212724> Keep `increasePacheCache` (see argument for `increaseRSS` above). - Benjamin Bannier On Aug. 18, 2016, 7:39 p.m., Jie Yu wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/51215/ > ----------------------------------------------------------- > > (Updated Aug. 18, 2016, 7:39 p.m.) > > > Review request for mesos and Benjamin Bannier. > > > Bugs: MESOS-6053 > https://issues.apache.org/jira/browse/MESOS-6053 > > > Repository: mesos > > > Description > ------- > > Moved memory test helper to the common test helper. > > > Diffs > ----- > > src/Makefile.am d389e02584cfb1a00067cd30de1364118d1c46c7 > src/tests/CMakeLists.txt 1ea8b2102753ae294bd75706ffaf08308e928acd > src/tests/cmake/MesosTestsConfigure.cmake > 361032082c3a5f76b949dc7981f75b53e02d84f5 > src/tests/containerizer/CMakeLists.txt > 2c52e43a9deee90fa32693731d6ebedb5201bb1f > src/tests/containerizer/memory_test_helper.hpp > 86a9b57c1872b2efe799b0dc61f3d3d73e4d134b > src/tests/containerizer/memory_test_helper.cpp > e850b376ac61e2dedb07ea418cfeaf400cbba172 > src/tests/test_helper_main.cpp PRE-CREATION > > Diff: https://reviews.apache.org/r/51215/diff/ > > > Testing > ------- > > > Thanks, > > Jie Yu > >
