----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4167/#review5605 -----------------------------------------------------------
src/slave/http.cpp <https://reviews.apache.org/r/4167/#comment12223> Why would we bother adding this before it does anything useful? src/slave/process_based_isolation_module.cpp <https://reviews.apache.org/r/4167/#comment12241> Make sure this entry exists and fail gracefully if it doesn't. (It's probably possible for an executor to die between the time usage is requested and when the ProcessInfo is deleted.) src/slave/resource_monitor.cpp <https://reviews.apache.org/r/4167/#comment12242> I assume that you've discussed calling this 'mem_usage' and 'cpu_usage' rather than 'mem' and 'cpus' with Ben? Can you explain the reasoning briefly? src/slave/slave.cpp <https://reviews.apache.org/r/4167/#comment12243> How is this deleted? Does this actually need to be on the heap? src/slave/slave.cpp <https://reviews.apache.org/r/4167/#comment12244> Suggest foreachpair src/slave/slave.cpp <https://reviews.apache.org/r/4167/#comment12245> const UsageMessage& - Charles On 2012-03-04 20:40:08, Sam Whitlock wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/4167/ > ----------------------------------------------------------- > > (Updated 2012-03-04 20:40:08) > > > Review request for mesos, Benjamin Hindman and Charles Reiss. > > > Summary > ------- > > This mega-patch is intended to represent the partial completion of the slave > monitoring functionality. It is not intended to be committed. Changes based > on comments in this review will be reflected in future reviews that are > smaller and more modular. > > Proc utils is included in this patch, but is already under review here: > https://reviews.apache.org/r/3050/ > > The relevant design doc can be found here: > https://docs.google.com/document/d/14Wj9i6TpMR6cV3LL0ySjLjfZOq5QOQeybvt1gSyaQGs/edit > > The following items are ones where specific feedback is requested: > > * A better mechanism is needed to control the rate at which the slave asks > each executor for its UsageMessage. This is currently hard-coded to be at 1 > second intervals, but could potentially be read as a command-line option or > from a config file. Is there a better or different way to pass in this value? > * Currently, UsageMessages are passed from a ResourceMonitor to the Slave > using the Future construct, and used as containers that hold a snapshot of > the latest usage. This is to prevent unnecessary marshalling and extra data > structures, since messages will eventually be sent in the standard dispatch > style from the slave to the master. Is it fine that we are using Protobuf > messages in this way? > > There are several changes that are not yet implemented in this patch. These > changes are as follows: > > * Sufficient tests cases have not yet been written for any component > (resource monitor, lxc collector, and process collector). > * Code has not been cleaned up to adhere to all style recommendations. > * Process collector code needs to be updated to prevent CPU usage spikes when > monitored sub-processes die. > * Code to send UsageMessages from the slave to the master. > > > This addresses bug MESOS-38. > https://issues.apache.org/jira/browse/MESOS-38 > > > Diffs > ----- > > src/monitoring/process_stats.hpp PRE-CREATION > src/monitoring/resource_collector.hpp PRE-CREATION > src/slave/http.cpp f03815d > src/slave/isolation_module.hpp c896908 > src/slave/isolation_module.cpp 5b7b4a2 > src/slave/lxc_isolation_module.hpp b7beefe > src/slave/lxc_isolation_module.cpp d544625 > src/slave/main.cpp ac780c4 > src/slave/process_based_isolation_module.hpp f6f9554 > src/slave/process_based_isolation_module.cpp 100b1e3 > src/slave/resource_monitor.hpp PRE-CREATION > src/slave/resource_monitor.cpp PRE-CREATION > src/slave/slave.hpp b1a07e9 > src/slave/slave.cpp ce8fda5 > src/tests/Makefile.in 6f51be4 > src/tests/proc_utils_tests.cpp PRE-CREATION > src/tests/process_resource_collector_tests.cpp PRE-CREATION > src/tests/resource_monitor_tests.cpp PRE-CREATION > src/monitoring/process_resource_collector.cpp PRE-CREATION > src/monitoring/process_resource_collector.hpp PRE-CREATION > src/monitoring/linux/proc_utils.cpp PRE-CREATION > src/monitoring/linux/proc_utils.hpp PRE-CREATION > src/monitoring/linux/proc_resource_collector.cpp PRE-CREATION > src/monitoring/linux/proc_resource_collector.hpp PRE-CREATION > src/monitoring/linux/lxc_resource_collector.cpp PRE-CREATION > src/master/master.hpp 53551b0 > src/master/master.cpp 1d3961e > src/messages/messages.proto 11a2c41 > src/monitoring/linux/lxc_resource_collector.hpp PRE-CREATION > src/master/allocator.hpp 1ac435b > src/master/http.cpp 591433a > src/Makefile.am 1137a3e > > Diff: https://reviews.apache.org/r/4167/diff > > > Testing > ------- > > Test cases: > * A test case exercising the basic monitoring code with a mocked-out > collector. > * The first of several tests for the process resource monitor, with the > proc-based collecting mocked out. > > Some ad-hoc testing with log statements to ensure that the monitoring works > end-to-end from both the container-based and process-based isolation modules. > > > Thanks, > > Sam > >
