CASSANDRA-15909 Metrics naming consistency #2
Conversation
spmallette
force-pushed
the
CASSANDRA-15909
branch
5 times, most recently
from
4554c95 to
eff2dc7
Compare
Deprecated the old names by simply renaming the metrics but then creating a secondary alias to the same metric instance.
spmallette
force-pushed
the
CASSANDRA-15909
branch
from
eff2dc7 to
13f8df5
Compare
| import java.util.Collections; | ||
| import java.util.Locale; | ||
| import java.util.Map; | ||
| import java.util.Objects; |
| return registerGauge(name, null, gauge); | ||
| } | ||
|
|
||
| private <T> Gauge<T> registerGauge(String name, String deprecated, Gauge<T> gauge) |
There was a problem hiding this comment.
nit: I'd like to avoid the "deprecated" terminology in the actual code (as opposed to in comments, where we can give hints about why aliases exist) in favor of "alias" in as many places as possible. (i.e. We have aliases, and they may exist for a number of reasons, including deprecation?) I'll expand on this in another comment in TableMetrics...
There was a problem hiding this comment.
yes - i wasn't a fan of "deprecated" either.......i wasn't sure how much tolerance folks had though for more drastic changes here. i expected that could be discussed during review. i don't quite remember what else i had in mind though.
There was a problem hiding this comment.
This one I think we can just change to alias. The stuff in TableMetrics (see comments there) is a more involved, but not that much more.
| * | ||
| * @param deprecated The name of the deprecated metric cannot be {@code null} | ||
| */ | ||
| protected <G,T> Gauge<T> createTableGauge(String name, String alias, String deprecated, Gauge<T> gauge, |
There was a problem hiding this comment.
Is there any caller of this new createTableGauge() overload where name and alias aren't identical.
| protected <G,T> Gauge<T> createTableGauge(String name, String alias, String deprecated, Gauge<T> gauge, | ||
| Gauge<G> globalGauge) | ||
| { | ||
| assert deprecated != null; |
There was a problem hiding this comment.
nit: If this is a hard pre-condition, perhaps Guava Preconditions would be a better fit. (I can't remember if OSS allows disabling assertions...)
| aliasFactory.createMetricName(alias), | ||
| factory.createMetricName(deprecated), | ||
| aliasFactory.createMetricName(deprecated)); | ||
| if (register(name, alias, deprecated, cfGauge) && globalGauge != null) |
There was a problem hiding this comment.
Does any caller actually pass a null globalGauge? Perhaps this is also better off with a precondition and not a silent failure to register the globals.
| return createTableTimer(name, alias, null, keyspaceTimer); | ||
| } | ||
|
|
||
| protected TableTimer createTableTimer(String name, String alias, String deprecated, Timer keyspaceTimer) |
There was a problem hiding this comment.
Another case here where I don't think name and alias are ever not identical, and the createTableTimer(String name, String alias, Timer keyspaceTimer) overload essentially should just go away.
| } | ||
|
|
||
| private void releaseMetric(String metricName, String metricAlias) | ||
| private void releaseMetric(String metricName, String metricAlias, String metricDeprecated) |
There was a problem hiding this comment.
I'd like to rename these from String metricName, String metricAlias, String metricDeprecated to something like String tableMetricName, String cfMetricName, String tableMetricAlias. It would at least try to match the reality of where the three of them live.
| allTableMetrics.get(metricName).remove(metric); | ||
| Metrics.remove(name, alias); | ||
|
|
||
| CassandraMetricsRegistry.MetricName[] aliases = null == metricDeprecated ? new CassandraMetricsRegistry.MetricName[1] : new CassandraMetricsRegistry.MetricName[3]; |
There was a problem hiding this comment.
nit: I'd probably just make two calls to Metrics.remove() here rather than worry about array creation with a conditional size.
| * @param deprecated the deprecated name for the metric which should be {@code null} if no deprecation is required | ||
| * @return true if first time metric with that name has been registered | ||
| */ | ||
| private boolean register(String name, String alias, String deprecated, Metric metric) |
There was a problem hiding this comment.
Same as below, I'd consider a naming scheme like String tableMetricName, String cfMetricName, String tableMetricAlias.
|
I know it would blow up the diff, but it would be really nice if we could name the |
|
@spmallette I've created apache#728, so you can close this if you like. |
https://issues.apache.org/jira/browse/CASSANDRA-15909