> On April 9, 2014, 1:02 p.m., Ben Mahler wrote:
> > I would like us to further explore removing the explicit 'add' and 'remove'
> > functions, as it would provide a huge benefit to users of Metrics. They
> > would no longer need to manually manage lifecycle of the Metrics. Is there
> > a concrete use case that we have to need explicit lifecycle management of a
> > Metric, outside of relying on the lifecycle of the Metric object itself?
> >
> > Here is a suggestion for eliminating 'add' and 'remove':
> >
> > I will use Counter as an example, but each derived Metric would need to do
> > the same. The following is just pseudo-code to illustrate:
> >
> > Counter(...)
> > : data(...)
> > {
> > // Refcount = 1;
> > add(context, name, new Counter(*this)); // Refcount = 2;
> > }
> >
> > ~Counter()
> > {
> > // Refcount >= 2.
> > data.clear();
> > // Refcount >= 1.
> > remove(context, name);
> > }
> >
> > MetricsProcess::remove(context, name)
> > {
> > // Are we holding the only copy?
> > if (metrics.contains(context) &&
> > metrics[context].contains(name) &&
> > metrics[context][name]->unique()) {
> > metrics[context].erase(name);
> > }
> > }
> >
The 'add' and 'remove' would be the responsibility of the derived class, which
is somewhat fragile. The 'unique' check should be fairly straightforward to
implement in Metric which (in later reviews) becomes copyable with shared data
too. As long as the derived class implements copying correctly everything
should work.
We don't have a concrete use case for separating the lifecycle from the
endpoint exposure, but it's feature we get by omitting potentially fragile code
and matches the two-phase construction we use elsewhere (Slave, Master).
We need a decision on this soon so I can finalize the patch. I understood that
we'd agreed on providing the explicit add and remove methods because it is less
fragile for people who want to create their own Metrics. We could also remove
the explicit add/remove in a future patch.
> On April 9, 2014, 1:02 p.m., Ben Mahler wrote:
> > 3rdparty/libprocess/include/process/metrics/counter.hpp, lines 39-44
> > <https://reviews.apache.org/r/18718/diff/21/?file=553929#file553929line39>
> >
> > What does postfix increment / decrement mean in the face of having
> > copyable shared-data Counters?
> >
> > Counter c(*this);
> > ++(*this); // Increments c as well!
> > return c;
> >
> > Maybe we should not be exposing postfix since it could be pretty
> > confusing in that it doesn't fit the typical postfix semantics very nicely.
I exposed them as it is expected that if you have pre, you have post. Given
that a user can already do something like:
Counter c;
Counter d = c;
++c;
and 'd' would also be incremented, I don't see that the post-fix operator adds
any extra confusion.
> On April 9, 2014, 1:02 p.m., Ben Mahler wrote:
> > 3rdparty/libprocess/include/process/metrics/metric.hpp, line 15
> > <https://reviews.apache.org/r/18718/diff/21/?file=553931#file553931line15>
> >
> > Have you considered just calling this 'value'? Snapshot sounds a bit
> > opaque as to what this function is to be used for.
> >
> > What is the purpose of this being an Option<double>? That part is not
> > clear and seems unnecessary, when would None() be used?
I considered 'snapshot', 'current' and 'value'. Value is too generic and
'snapshot' was already in use as terminology when I started talking to people.
I'm happy to change this if we can get consensus ;)
Option is necessary for Gauges. For instance, the system metrics sometimes
return 'None' when a value is not available.
> On April 9, 2014, 1:02 p.m., Ben Mahler wrote:
> > 3rdparty/libprocess/include/process/metrics/metrics.hpp, line 29
> > <https://reviews.apache.org/r/18718/diff/21/?file=553932#file553932line29>
> >
> > Does this need to be public?
> >
> > Why the change from 'snapshot' to 'all'? If we change the 'snapshot'
> > method to 'value', then the endpoint can retain it's name of 'snapshot'
> > right?
> >
> > It seems nice to capture that we're providing a snapshot view of metric
> > values via this particular endpoint. Especially as history comes into play
> > in the future.
Given that we have one endpoint as a requirement, snapshot didn't match what
this was returning. If Metric::snapshot becomes Metric::current or value (i
prefer current) then yes, this can be 'snapshot'.
> On April 9, 2014, 1:02 p.m., Ben Mahler wrote:
> > 3rdparty/libprocess/include/process/metrics/metrics.hpp, line 47
> > <https://reviews.apache.org/r/18718/diff/21/?file=553932#file553932line47>
> >
> > This Metric pointer is owned, correct? We should at least add a comment
> > mentioning this or make this an explicit 'Owned' if possible, noting that
> > the Metric is copied.
Owned is our equivalent of unique_ptr, even though it's shared under the hood,
right? I wasn't clear on when to use it given the shared implementation.
> On April 9, 2014, 1:02 p.m., Ben Mahler wrote:
> > 3rdparty/libprocess/include/process/metrics/metric.hpp, lines 35-36
> > <https://reviews.apache.org/r/18718/diff/21/?file=553931#file553931line35>
> >
> > Can you kill the _ suffixes?
in this case, i need something so i can retain the context()/name() accessors.
However, a future patch removes them again (20015).
> On April 9, 2014, 1:02 p.m., Ben Mahler wrote:
> > 3rdparty/libprocess/src/metrics/metrics.cpp, lines 88-91
> > <https://reviews.apache.org/r/18718/diff/21/?file=553933#file553933line88>
> >
> > Do you need to explicitly construct 'values' or can you just pass
> > metrics.values() to await()?
I had to take a local copy for some reason - passing it directly gave an error
about no such 'await' method. However, i don't need the loop.
- Dominic
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/18718/#review39921
-----------------------------------------------------------
On April 9, 2014, 2:30 p.m., Dominic Hamon wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/18718/
> -----------------------------------------------------------
>
> (Updated April 9, 2014, 2:30 p.m.)
>
>
> Review request for mesos, Benjamin Hindman and Ben Mahler.
>
>
> Bugs: MESOS-1036
> https://issues.apache.org/jira/browse/MESOS-1036
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> See summary
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/Makefile.am c785c4dd852eacaec1be11c68e0b0b95a328d96b
> 3rdparty/libprocess/include/process/metrics/counter.hpp PRE-CREATION
> 3rdparty/libprocess/include/process/metrics/gauge.hpp PRE-CREATION
> 3rdparty/libprocess/include/process/metrics/metric.hpp PRE-CREATION
> 3rdparty/libprocess/include/process/metrics/metrics.hpp PRE-CREATION
> 3rdparty/libprocess/src/metrics/metrics.cpp PRE-CREATION
> 3rdparty/libprocess/src/tests/metrics_tests.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/18718/diff/
>
>
> Testing
> -------
>
> Added unit tests. make check.
>
>
> Thanks,
>
> Dominic Hamon
>
>
