Allow subclasses to add metrics to MetricsRegistry

[chids]

This is primarily for testing purposes: being able to subclass MetricsRegistry and use getOrAdd() from the subclass to add manually constructed metrics (and possibly mock objects).

I made getOrAdd() final since I feels its logic is an integral part of MetricsRegistry that you shouldn't tamper with it. Might be to conservative?

Also took the opportunity to use getOrAdd in newMeter and newTimer.

[codahale]

I'm OK with making getOrAdd() protected, but the reason for newMeter and newTimer having different logic is that it tries harder to not unnecessarily create duplicate timers or meters. The tick threads keep them around until you stop them, which can lead to memory leaks.

[chids]

Yep, but does this patch actually change the behaviour for adding Timer/Meters? I at least thought I only removed duplication (I'm not in front of a computer right now).

/mårten.

On 17 nov 2011, at 23:57, Coda Halereply@reply.github.com wrote:

I'm OK with making getOrAdd() protected, but the reason for newMeter and newTimer having different logic is that it tries harder to not unnecessarily create duplicate timers or meters. The tick threads keep them around until you stop them, which can lead to memory leaks.

---

Reply to this email directly or view it on GitHub:
https://github.com/codahale/metrics/pull/112#issuecomment-2783345

[codahale]

It went from creating the metric after checking to creating the metric before checking. I've got one more fix for this and it should be fine.

[chids]

Either I'm way too tired or we're not reading the same diff ;) Since I didn't touch/reorder the line creating instance I fail to see how that could be the case

On 18 nov 2011, at 00:17, Coda Halereply@reply.github.com wrote:

It went from creating the metric after checking to creating the metric before checking. I've got one more fix for this and it should be fine.

---

Reply to this email directly or view it on GitHub:
https://github.com/codahale/metrics/pull/112#issuecomment-2783552

[codahale]

When getOrAdd is called, its metric parameter is already created. So newTimer went from this:

final Metric existingMetric = metrics.get(metricName);
if (existingMetric == null) {
    final TimerMetric metric = new TimerMetric(newMeterTickThreadPool(), durationUnit, rateUnit);
    // etc
}

to (effectively) this:

final TimerMetric metric = new TimerMetric(newMeterTickThreadPool(), durationUnit, rateUnit);
final Metric existingMetric = metrics.get(metricName);
if (existingMetric == null) {
    // etc.
}

[chids]

The check (line 285 to 287 in the diff) is untouched and thus performed before getOrAdd and before creating the metric instance.

I noticed now however that my patch caused the check to be carried out twice. Once in newXYZ (before creating the metric) and once in getOrAdd (when the metric have been created).

On 18 nov 2011, at 00:36, Coda Halereply@reply.github.com wrote:

When getOrAdd is called, its metric parameter is already created. So newTimer went from this:

final Metric existingMetric = metrics.get(metricName);
if (existingMetric == null) {
   final TimerMetric metric = new TimerMetric(newMeterTickThreadPool(), durationUnit, rateUnit);
   // etc
}

to (effectively) this:

final TimerMetric metric = new TimerMetric(newMeterTickThreadPool(), durationUnit, rateUnit);
final Metric existingMetric = metrics.get(metricName);
if (existingMetric == null) {
   // etc.
}

---

Reply to this email directly or view it on GitHub:
https://github.com/codahale/metrics/pull/112#issuecomment-2783699

[codahale]

Ah well, so it did.

sigh

I really need to stop trying to read diffs on GitHub.

Sorry about that.

[chids]

No worries. You really had me staring down those lines of code trying to figure out what I'd missed :D

On 18 nov 2011, at 02:07, Coda Halereply@reply.github.com wrote:

Ah well, so it did.

sigh

I really need to stop trying to read diffs on GitHub.

Sorry about that.

---

Reply to this email directly or view it on GitHub:
https://github.com/codahale/metrics/pull/112#issuecomment-2784481