----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30546/#review77820 -----------------------------------------------------------
Looks great Chi, can you adjust this per my comments on the depedent review about renaming the "Listener" to "Counter"? include/mesos/mesos.proto <https://reviews.apache.org/r/30546/#comment126116> It wasn't obvious to me initially that these reset to 0 when the slave restarts, could we add a brief note about that? src/slave/containerizer/isolators/cgroups/mem.cpp <https://reviews.apache.org/r/30546/#comment126128> Brace on the next line, also, why the trailing semi-colon? src/slave/containerizer/isolators/cgroups/mem.cpp <https://reviews.apache.org/r/30546/#comment126117> Could you remove the trailing period here? Also, any reason you're not including the listener.error() in the message? Seems useful. src/slave/containerizer/isolators/cgroups/mem.cpp <https://reviews.apache.org/r/30546/#comment126125> It looks like the isolators have become inconsistent about what they return for an unknown container (e.g. network isolator is not returning a failure for unknown container, but the memory/cpu isolators are). Much like in `usage`, let's just return a Failure here rather than an empty result. The containerizer should understand that it destroyed this container and doesn't need the usage() result anymore, right? src/slave/containerizer/isolators/cgroups/mem.cpp <https://reviews.apache.org/r/30546/#comment126123> Can you indent the cases? Also, there is a bug here, I'll let you find it ;) src/slave/containerizer/isolators/cgroups/mem.cpp <https://reviews.apache.org/r/30546/#comment126122> Not returning a Failure in this case (like the file parsing cases in `usage()` above) seems to suggest that we're not sure if the pressure counters can fail and so we want to proceed with the partial result. That's good, but why not keep logging the error? (rather than erasing the counter upon the first issue we see). Since we're not expecting this to be possible (but we're not confident enough about eventfds to make this a Failure()) let's just print an ERROR each time we are returning a partial result, to make this more obvious in the logs. This also ends up cleaning up the code, as you no longer need to keep an iterator around for erasing. src/slave/containerizer/isolators/cgroups/mem.cpp <https://reviews.apache.org/r/30546/#comment126127> Do you need to print this? Seems unnecessary? - Ben Mahler On March 25, 2015, 12:03 a.m., Chi Zhang wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/30546/ > ----------------------------------------------------------- > > (Updated March 25, 2015, 12:03 a.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 3c592d5ab3092ecbeddfaff95e0c1addc3ac58f8 > src/slave/containerizer/isolators/cgroups/mem.hpp > a00f723de61b9bccd76a2948b6d14fde7a993a8d > src/slave/containerizer/isolators/cgroups/mem.cpp > eaeb30164e8ae8c0a703683b7cc0bf1851760c95 > src/tests/slave_recovery_tests.cpp 87f4a6aab27d142fa8eb7a6571f684a6ce59687e > > Diff: https://reviews.apache.org/r/30546/diff/ > > > Testing > ------- > > > Thanks, > > Chi Zhang > >
