----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4167/#review5603 -----------------------------------------------------------
There are a couple comments we forgot to add about the work that still needs to be done. 1. We have not yet done any benchmarking to evaluate the performance cost of this additional monitoring. 2. Another configuration option to consider is the ability to enable/disable monitoring. Perhaps a configured sample rate of 0 should disable monitoring? - Alex On 2012-03-04 01:15:55, Sam Whitlock wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/4167/ > ----------------------------------------------------------- > > (Updated 2012-03-04 01:15:55) > > > 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/Makefile.am 1137a3e > src/master/allocator.hpp 1ac435b > src/master/http.cpp 591433a > 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/monitoring/linux/lxc_resource_collector.cpp PRE-CREATION > src/monitoring/linux/proc_resource_collector.hpp PRE-CREATION > src/monitoring/linux/proc_resource_collector.cpp PRE-CREATION > src/monitoring/linux/proc_utils.hpp PRE-CREATION > src/monitoring/linux/proc_utils.cpp PRE-CREATION > src/monitoring/process_resource_collector.hpp PRE-CREATION > src/monitoring/process_resource_collector.cpp PRE-CREATION > 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/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 > > 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 > >
