> 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);
> > }
> > }
> >
>
> Dominic Hamon wrote:
> 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.
>
> Ben Mahler wrote:
> Let's explore what 'fragility' means here.
>
> Option 1: Only Metric lifetime to deal with, no explicit 'add'/'remove'
> lifecycle.
> Here, we already require that each derived Metric is shared and copyable.
> So we're adding some required lifecycle management built-in to the derived
> Metric. We only have to get the lifecycle management right in the library for
> all uses of the Metrics to be considered valid.
>
> Option 2: Callers must explicitly manage lifecycle with 'add'/'remove'.
> Here, each caller must get the lifecycle management correct. If they do
> not, they either do not have their metrics managed or they "leak" metric
> objects and we have unbounded growth of metrics.
>
> One could argue that both options have their fragile aspects, in option 1
> the lifecycle has to be managed correctly in the library itself. For option
> 2, the lifecycle has to be managed correctly in each caller. I would be more
> inclined to eliminate the ability for callers to make a mistake, and have the
> library provide the lifecycle management, if possible.
I was thinking that option 1 also requires that someone deriving a new metric
copies *this in the call to 'add', however that's not the case as your pseudo
code would actually be:
Counter(...) : data(...) { add(*this); }
It's unfortunate that the deriver would have to remember to call data.reset()
before calling remove, though if they forget this the metric would just remain
in the endpoint.
If you think this is a reasonable potential cost then I'm happy to go along
with the implicit lifetime management.
- Dominic
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/18718/#review39921
-----------------------------------------------------------
On April 9, 2014, 4:16 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, 4:16 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
>
>
