> On Jan. 21, 2015, 1:55 p.m., Ben Mahler wrote: > > src/slave/monitor/monitor.hpp, line 44 > > <https://reviews.apache.org/r/30090/diff/1/?file=827306#file827306line44> > > > > 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!
I renamed based on your suggestion. Thanks for picking this up. > On Jan. 21, 2015, 1:55 p.m., Ben Mahler wrote: > > src/slave/monitor/monitor.cpp, line 40 > > <https://reviews.apache.org/r/30090/diff/1/?file=827307#file827307line40> > > > > This flag doesn't exist! Should this be depending on something? Is this > > chain created correctly? This gets introduced later and I realize that's super tricky to review; the chain needs to be applied atomically. I renamed the patches and they will all go in at the same time. Is that OK or would you like to see the new flag introduced earlier? > On Jan. 21, 2015, 1:55 p.m., Ben Mahler wrote: > > src/slave/monitor/monitor.hpp, lines 43-44 > > <https://reviews.apache.org/r/30090/diff/1/?file=827306#file827306line43> > > > > 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! Excellent suggestion (And TODO! :). I will create a JIRA for it. - Niklas ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30090/#review69013 ----------------------------------------------------------- On Jan. 21, 2015, 2:36 p.m., Niklas Nielsen wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/30090/ > ----------------------------------------------------------- > > (Updated Jan. 21, 2015, 2:36 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 > >
