----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30546/#review70779 -----------------------------------------------------------
Another high level review because I think there are important architectural issues first. include/mesos/mesos.proto <https://reviews.apache.org/r/30546/#comment116203> Specifically, they should be uint64. src/slave/containerizer/isolators/cgroups/mem.hpp <https://reviews.apache.org/r/30546/#comment116208> These statistics are best interpreted by their derivative so wrapping around isn't a problem, in fact, if the slave is restarted they'll restart from zero anyway. I'm more concerned that you're handing out a pointer to a member variable to another libprocess... src/slave/containerizer/isolators/cgroups/mem.cpp <https://reviews.apache.org/r/30546/#comment116222> This is giving the address to a member variable to who...? src/tests/isolator_tests.cpp <https://reviews.apache.org/r/30546/#comment116228> This isn't really testing "memory pressure" since you're just allocating your entire allowance. I suggest performing IO to completely fill up the page cache then start allocating anonymous pages, this will give pressure without 'exploding' ;-) src/tests/isolator_tests.cpp <https://reviews.apache.org/r/30546/#comment116224> What about putting the tests for specific expectations into the tests for the memory pressure code, not the isolator that uses it? See my comments in the other review. - Ian Downes On Feb. 2, 2015, 9:29 p.m., Chi Zhang wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/30546/ > ----------------------------------------------------------- > > (Updated Feb. 2, 2015, 9:29 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 > ------- > > MemIsolator: expose memory pressures for containers. > > > Diffs > ----- > > include/mesos/mesos.proto 26003fada2e4c4be0e9ebc4663f7ebab858cc48d > src/slave/containerizer/isolators/cgroups/mem.hpp > 2fa755571b4d21b8b13301fcfd57ae05ea66e6e6 > src/slave/containerizer/isolators/cgroups/mem.cpp > 711d66d7771cac13be831d73af3ef570d6785473 > src/tests/isolator_tests.cpp 1f1c26d4a8faf6fcea822fccc00bb58a478285f8 > > Diff: https://reviews.apache.org/r/30546/diff/ > > > Testing > ------- > > > Thanks, > > Chi Zhang > >
