You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mesos.apache.org by Niklas Nielsen <ni...@qni.dk> on 2015/01/20 22:38:19 UTC
Review Request 30090: Added abstract interface for resource monitor.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30090/
-----------------------------------------------------------
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
Re: Review Request 30090: Monitor modularization [2/8]: Added abstract
interface for resource monitor.
Posted by Niklas Nielsen <ni...@qni.dk>.
> 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
>
>
Re: Review Request 30090: Added abstract interface for resource
monitor.
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
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
>
>
Re: Review Request 30090: Monitor modularization [2/8]: Added abstract
interface for resource monitor.
Posted by Niklas Nielsen <ni...@qni.dk>.
> On Jan. 20, 2015, 1:59 p.m., Kapil Arya wrote:
> > src/slave/monitor/monitor.hpp, lines 38-40
> > <https://reviews.apache.org/r/30090/diff/1/?file=827306#file827306line38>
> >
> > Is there a reason for not doing a #include for containerizer.hpp and slave/flags.hpp?
Nope - good catch. Copy-pasted from the old monitor code.
- Niklas
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30090/#review68786
-----------------------------------------------------------
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
>
>
Re: Review Request 30090: Added abstract interface for resource
monitor.
Posted by Niklas Nielsen <ni...@qni.dk>.
> On Jan. 20, 2015, 1:59 p.m., Kapil Arya wrote:
> > src/slave/monitor/monitor.hpp, lines 19-20
> > <https://reviews.apache.org/r/30090/diff/1/?file=827306#file827306line19>
> >
> > Should we start using `#pragma once` instead? I have been using that in a different patchset.
There is still not consensus for that change yet. Let's reopen if that changes before this lands.
> On Jan. 20, 2015, 1:59 p.m., Kapil Arya wrote:
> > src/slave/monitor/monitor.cpp, lines 21-23
> > <https://reviews.apache.org/r/30090/diff/1/?file=827307#file827307line21>
> >
> > Rearrange as:
> > ```
> > #include "slave/flags.hpp"
> >
> > #include "slave/monitor/monitor.hpp"
> >
> > #include "slave/monitor/mesos/monitor.hpp"
> > ```
> >
> > Since slave/monitor is a subdirectory and so is slave/monitor/mesos
We don't have consistent rules around spacing header includes with regards to subfolders. See for example: https://github.com/apache/mesos/blob/master/src/slave/slave.hpp#L50
I'd be happy to file a JIRA and get it in the style guide. Dropping for now.
> On Jan. 20, 2015, 1:59 p.m., Kapil Arya wrote:
> > src/slave/monitor/monitor.cpp, line 45
> > <https://reviews.apache.org/r/30090/diff/1/?file=827307#file827307line45>
> >
> > Kill this newline?
Good point! Thanks :)
> On Jan. 20, 2015, 1:59 p.m., Kapil Arya wrote:
> > src/slave/monitor/monitor.cpp, line 56
> > <https://reviews.apache.org/r/30090/diff/1/?file=827307#file827307line56>
> >
> > Since it's already a `Try<>` type, can't we just do a `return monitor;` here?
Good catch - yup, can just return monitor.
- Niklas
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30090/#review68786
-----------------------------------------------------------
On Jan. 20, 2015, 1: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, 1: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
>
>
Re: Review Request 30090: Added abstract interface for resource
monitor.
Posted by Kapil Arya <ka...@mesosphere.io>.
> On Jan. 20, 2015, 4:59 p.m., Kapil Arya wrote:
> > src/slave/monitor/monitor.hpp, lines 47-53
> > <https://reviews.apache.org/r/30090/diff/1/?file=827306#file827306line47>
> >
> > I don't think modules impose any rule on constructor with/without arguments. The only concern from the point of view of modules is the fact that doing an `bool initialize(...)` allows us to realize failures and we can return a NULL from the `create()` method.
> >
> > If it looks like the initialize() method will never fail (as is the case with this use case), you should feel free to call constructor with arguments.
I misread the code/comments. My comment/issue is not relevant here.
- Kapil
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30090/#review68786
-----------------------------------------------------------
On Jan. 20, 2015, 4: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, 4: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
>
>
Re: Review Request 30090: Added abstract interface for resource
monitor.
Posted by Kapil Arya <ka...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30090/#review68786
-----------------------------------------------------------
src/slave/monitor/monitor.hpp
<https://reviews.apache.org/r/30090/#comment113236>
Should we start using `#pragma once` instead? I have been using that in a different patchset.
src/slave/monitor/monitor.hpp
<https://reviews.apache.org/r/30090/#comment113239>
Is there a reason for not doing a #include for containerizer.hpp and slave/flags.hpp?
src/slave/monitor/monitor.hpp
<https://reviews.apache.org/r/30090/#comment113240>
I don't think modules impose any rule on constructor with/without arguments. The only concern from the point of view of modules is the fact that doing an `bool initialize(...)` allows us to realize failures and we can return a NULL from the `create()` method.
If it looks like the initialize() method will never fail (as is the case with this use case), you should feel free to call constructor with arguments.
src/slave/monitor/monitor.hpp
<https://reviews.apache.org/r/30090/#comment113241>
Same as above, use `#pragma once` instead.
src/slave/monitor/monitor.cpp
<https://reviews.apache.org/r/30090/#comment113242>
Rearrange as:
```
#include "slave/flags.hpp"
#include "slave/monitor/monitor.hpp"
#include "slave/monitor/mesos/monitor.hpp"
```
Since slave/monitor is a subdirectory and so is slave/monitor/mesos
src/slave/monitor/monitor.cpp
<https://reviews.apache.org/r/30090/#comment113244>
Kill this newline?
src/slave/monitor/monitor.cpp
<https://reviews.apache.org/r/30090/#comment113243>
Since it's already a `Try<>` type, can't we just do a `return monitor;` here?
- Kapil Arya
On Jan. 20, 2015, 4: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, 4: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
>
>
Re: Review Request 30090: Monitor modularization [2/8]: Added abstract
interface for resource monitor.
Posted by Niklas Nielsen <ni...@qni.dk>.
-----------------------------------------------------------
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.
Summary (updated)
-----------------
Monitor modularization [2/8]: Added abstract interface for resource monitor.
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