-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/18718/#review39921
-----------------------------------------------------------
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);
}
}
3rdparty/libprocess/include/process/metrics/counter.hpp
<https://reviews.apache.org/r/18718/#comment72678>
Move this up?
3rdparty/libprocess/include/process/metrics/counter.hpp
<https://reviews.apache.org/r/18718/#comment72676>
Seems nice to be explicit here and in ~Gauge and declare these virtual,
rather than rely on the implicit virtual.
3rdparty/libprocess/include/process/metrics/counter.hpp
<https://reviews.apache.org/r/18718/#comment72687>
This needs to call the Base class' assignment operator as well, no?
3rdparty/libprocess/include/process/metrics/counter.hpp
<https://reviews.apache.org/r/18718/#comment72668>
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.
3rdparty/libprocess/include/process/metrics/counter.hpp
<https://reviews.apache.org/r/18718/#comment72673>
It looks like -=/decrement and +=/increment are redundant, can you move the
decrement/increment functionality into the -=/+= operators?
If so, then the prefix operators (if we want them) are simplified as well,
right?
Counter& operator -- () { return (*this) += 1; }
3rdparty/libprocess/include/process/metrics/counter.hpp
<https://reviews.apache.org/r/18718/#comment72675>
See duration.hpp or bytes.hpp for how we format these operators.
3rdparty/libprocess/include/process/metrics/counter.hpp
<https://reviews.apache.org/r/18718/#comment72665>
Should these and all other methods be taking 'int64_t'?
3rdparty/libprocess/include/process/metrics/gauge.hpp
<https://reviews.apache.org/r/18718/#comment72679>
Move this up?
3rdparty/libprocess/include/process/metrics/gauge.hpp
<https://reviews.apache.org/r/18718/#comment72686>
This needs to call the Base class' assignment operator as well, no?
3rdparty/libprocess/include/process/metrics/metric.hpp
<https://reviews.apache.org/r/18718/#comment72683>
Maybe a brief comment on Metric / Gauge / Counter? Gauge would benefit the
most from a brief comment as I recall people being confused with the
terminology.
3rdparty/libprocess/include/process/metrics/metric.hpp
<https://reviews.apache.org/r/18718/#comment72680>
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?
3rdparty/libprocess/include/process/metrics/metric.hpp
<https://reviews.apache.org/r/18718/#comment72681>
Can you kill the _ suffixes?
3rdparty/libprocess/include/process/metrics/metrics.hpp
<https://reviews.apache.org/r/18718/#comment72689>
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.
3rdparty/libprocess/include/process/metrics/metrics.hpp
<https://reviews.apache.org/r/18718/#comment72690>
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.
3rdparty/libprocess/src/metrics/metrics.cpp
<https://reviews.apache.org/r/18718/#comment72692>
Do you need to explicitly construct 'values' or can you just pass
metrics.values() to await()?
- Ben Mahler
On April 9, 2014, 5:05 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, 5:05 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
>
>
