mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Benjamin Mahler <bmah...@apache.org>
Subject Re: Review Request 59858: Allow Gauge metrics to avoid process::defer().
Date Thu, 08 Jun 2017 20:25:02 GMT


> On June 6, 2017, 10:18 p.m., Benjamin Bannier wrote:
> > 3rdparty/libprocess/include/process/metrics/gauge.hpp
> > Line 34 (original), 35-39 (patched)
> > <https://reviews.apache.org/r/59858/diff/2/?file=1743153#file1743153line35>
> >
> >     I believe the gist of this change is to recognize that a `Gauge` has no dependency
on the production of its value to be deferred; instead this decision should be should be left
to the creator of the `Gauge`.
> >     
> >     For most existing `Gauge`s in the Mesos code base we not only depend on some
process to carry out calculcation of values, but also introduce a life-time dependency made
safe by using `Deferred`. This is inherent to the functions passed in here (they could e.g.,
be implicitly converted from a `_Deferred`.
> >     
> >     Given that, I am not sure we need to explain `Deferred` here. I would instead
update the existing comment, e.g., `s/deferred object/producer function/` or similar.
> 
> James Peach wrote:
>     I wanted to capture something about the common usage in the codebase and give some
guidance without second-guessing how this should be used. I would like to make the comment
a bit more expansive so we can explain how to use `Gauge` safely, but I'm open to suggestions
:)
> 
> Benjamin Bannier wrote:
>     I cannot see how the caveats of function objects used with a `Gauge` are any different
from uses elsewhere (e.g., in continuations). If anything the previous version should have
explained why it choose that particular, restricted interface, but the form here seems natural
to me and requiring no references to possible data or lifetime dependencies.

I think we can document some of the "guidelines" or "patterns" here at the top level documentation
of `Gauge` (i.e. "defer" and "thread safe access to Process member data") because the latter
pattern is non-trivial, so I'm hesistant to put something like "the defer is not required"
until we actually show what needs to be done to safely avoid defer for accessing Process member
fields.

I think we should **at least** document the following w.r.t. to lifetime:

```
// The user of `Gauge` must ensure that `f` is safe to execute up until
// the removal of the `Gauge` (via `process::metrics::remove(...)`) is
// complete.
```

For now I will leave the defer details out, and we can follow up by documenting safe examples
at the top of `Gauge` (rather than on the constructor documentation for `f`). How does that
sound?


- Benjamin


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59858/#review177103
-----------------------------------------------------------


On June 6, 2017, 9:16 p.m., James Peach wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59858/
> -----------------------------------------------------------
> 
> (Updated June 6, 2017, 9:16 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier and Benjamin Mahler.
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> A `Deferred` function object is always defined to be a
> `std::function`, so when we are constructing a `Gauge` metrics
> is it not strictly required for the callback to be a `Deferred`
> object. In cases where the value is a constant or can be calculated
> by reading constant data, a metrics producer can emit a `Gauge`
> without the taking the additional dispatch cost.
> 
> 
> Diffs
> -----
> 
>   3rdparty/libprocess/include/process/metrics/gauge.hpp c5bbeaf5d53c60f3636d1778db47cdb6cdda34e8

>   3rdparty/libprocess/src/tests/metrics_tests.cpp c13520d23ca17144f553fb4588fb8a747ea46e72

> 
> 
> Diff: https://reviews.apache.org/r/59858/diff/3/
> 
> 
> Testing
> -------
> 
> make check (Fedora 25)
> 
> 
> Thanks,
> 
> James Peach
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message