----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30090/#review69013 -----------------------------------------------------------
src/slave/monitor/monitor.hpp <https://reviews.apache.org/r/30090/#comment113582> When we pull out interfaces like this, it would be great to document why it's pulled out. Could we also migrate my TODOs from the existing resource monitor? ``` // Defines the interface for performing resource monitoring on containers. // // TODO(nnielsen): Add the HTTP API to this interface to force a // minimal API across monitoring implementations. Currently the // MesosResourceMonitor implicitly provides the HTTP API. // // TODO(bmahler): Consider pulling out the resource collection into // a Collector abstraction that can be leveraged by the monitor. // The monitor can then only be responsible for history and actual // "monitoring". For example, isolators to subscribe to resource // usage events. (e.g. get a future for the executor hitting 75% // memory consumption, the future would become ready when this // occurs, and the isolator can discard the future when no longer // interested). // // TODO(bmahler): Consider forwarding usage information to the master. ``` I also added a bit TODO about ensuring other modules provide the minimal existing HTTP API for container metrics, otherwise using a module may break functionality for many users. A follow up on that would be great! src/slave/monitor/monitor.hpp <https://reviews.apache.org/r/30090/#comment113583> This patch introduces a naming conflict with the existing ResourceMonitor, since it can be avoided it would have been great do the rename to `MesosResourceMonitor` in the same patch, splitting that change seems more trouble than keeping them in the same patch. Accordingly, please atomically commit this along with the rest of your chain. Ideally a prefix in the commit message will help see the dependency for cherry-picks, etc: ``` Monitor modularization 1: Added ResourceMonitor interface Monitor modularization 2: ... ``` But in the future let's try to avoid naming conflicts like this! src/slave/monitor/monitor.hpp <https://reviews.apache.org/r/30090/#comment113584> period src/slave/monitor/monitor.cpp <https://reviews.apache.org/r/30090/#comment113599> This flag doesn't exist! Should this be depending on something? Is this chain created correctly? - Ben Mahler On Jan. 20, 2015, 9:38 p.m., Niklas Nielsen wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/30090/ > ----------------------------------------------------------- > > (Updated Jan. 20, 2015, 9:38 p.m.) > > > Review request for mesos and Ben Mahler. > > > Bugs: MESOS-2219 > https://issues.apache.org/jira/browse/MESOS-2219 > > > Repository: mesos-git > > > Description > ------- > > Added abstract interface for already existing monitor API. > > > Diffs > ----- > > src/slave/monitor/monitor.hpp PRE-CREATION > src/slave/monitor/monitor.cpp PRE-CREATION > > Diff: https://reviews.apache.org/r/30090/diff/ > > > Testing > ------- > > make check > > > Thanks, > > Niklas Nielsen > >
