> 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 > >
