You are viewing a plain text version of this content. The canonical link for it is here.
Posted to pr@cassandra.apache.org by GitBox <gi...@apache.org> on 2020/08/27 23:36:42 UTC
[GitHub] [cassandra] dcapwell commented on a change in pull request #728: CASSANDRA-15909 Make Table/Keyspace Metric Names Consistent With Each Other
dcapwell commented on a change in pull request #728:
URL: https://github.com/apache/cassandra/pull/728#discussion_r478751962
##########
File path: src/java/org/apache/cassandra/metrics/TableMetrics.java
##########
@@ -1143,23 +1098,29 @@ protected TableTimer createTableTimer(String name, Timer keyspaceTimer)
protected TableTimer createTableTimer(String name, String alias, Timer keyspaceTimer)
{
- Timer cfTimer = Metrics.timer(factory.createMetricName(name), aliasFactory.createMetricName(alias));
- register(name, alias, cfTimer);
- return new TableTimer(cfTimer,
- keyspaceTimer,
- Metrics.timer(globalFactory.createMetricName(name),
- globalAliasFactory.createMetricName(alias)));
+ return createTableTimer(name, alias, null, keyspaceTimer);
}
- protected Timer createTableTimer(String name)
+ protected TableTimer createTableTimer(String name, String alias, String deprecated, Timer keyspaceTimer)
{
- return createTableTimer(name, name);
+ Timer cfTimer = Metrics.timer(factory.createMetricName(name), aliasFactory.createMetricName(alias));
+ register(name, alias, deprecated, keyspaceTimer);
+ Timer global = Metrics.timer(globalFactory.createMetricName(name),
+ globalAliasFactory.createMetricName(alias));
+
+ if (deprecated != null)
+ {
+ Metrics.registerMBean(global, globalFactory.createMetricName(deprecated).getMBeanName());
+ Metrics.registerMBean(global, globalAliasFactory.createMetricName(deprecated).getMBeanName());
+ }
+
+ return new TableTimer(cfTimer, keyspaceTimer, global);
}
- protected Timer createTableTimer(String name, String alias)
+ protected Timer createTableTimer(String name)
{
- Timer tableTimer = Metrics.timer(factory.createMetricName(name), aliasFactory.createMetricName(alias));
- register(name, alias, tableTimer);
+ Timer tableTimer = Metrics.timer(factory.createMetricName(name), aliasFactory.createMetricName(name));
+ register(name, name, tableTimer);
Review comment:
alias will hit check for existence and no-op, so looks safe.
##########
File path: src/java/org/apache/cassandra/metrics/ClientMetrics.java
##########
@@ -136,16 +140,19 @@ private int countConnectedClients()
for (Server server : servers)
for (ClientStat stat : server.recentClientStats())
- stats.add(new HashMap(stat.asMap())); // asMap returns guava, so need to convert to java for jmx
+ stats.add(new HashMap<>(stat.asMap())); // asMap returns guava, so need to convert to java for jmx
stats.sort(Comparator.comparing(map -> map.get(ClientStat.PROTOCOL_VERSION)));
return stats;
}
- private <T> Gauge<T> registerGauge(String name, Gauge<T> gauge)
+ private <T> Gauge<T> registerGauge(String name, String deprecated, Gauge<T> gauge)
Review comment:
Thinking this would be cleaner to have two methods
```
private <T> Gauge<T> registerGauge(String name, String deprecated, Gauge<T> gauge)
{
return Metrics.register(factory.createMetricName(name), gauge, factory.createMetricName(deprecated));
}
private <T> Gauge<T> registerGauge(String name, Gauge<T> gauge)
{
return Metrics.register(factory.createMetricName(name), gauge);
}
```
current logic is mostly rewriting that, if one method is desired, would be best to create the `MetricName... aliases` in this method then.
##########
File path: src/java/org/apache/cassandra/metrics/KeyspaceMetrics.java
##########
@@ -233,8 +233,8 @@ public KeyspaceMetrics(final Keyspace ks)
confirmedRepairedInconsistencies = createKeyspaceMeter("RepairedDataInconsistenciesConfirmed");
unconfirmedRepairedInconsistencies = createKeyspaceMeter("RepairedDataInconsistenciesUnconfirmed");
- repairedDataTrackingOverreadRows = createKeyspaceHistogram("RepairedOverreadRows", false);
Review comment:
for me, this doesn't need to be aliased, see https://issues.apache.org/jira/browse/CASSANDRA-15909?focusedCommentId=17150195&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-17150195
##########
File path: src/java/org/apache/cassandra/metrics/TableMetrics.java
##########
@@ -1027,6 +953,35 @@ public Long getValue()
return cfGauge;
}
+ /**
+ * Same as {@link #createTableGauge(String, Gauge, Gauge)} but accepts a deprecated
+ * name for a table {@code Gauge}. Prefer that method when deprecation is not necessary.
+ *
+ * @param name the name of the metric registered with the "Table" type
+ * @param deprecated the deprecated name for the metric registered with the "Table" type
+ */
+ protected <G,T> Gauge<T> createTableGaugeWithDeprecation(String name, String deprecated, Gauge<T> gauge, Gauge<G> globalGauge)
+ {
+ assert deprecated != null : "no deprecated metric name provided";
+ assert globalGauge != null : "no global Gauge metric provided";
+
+ Gauge<T> cfGauge = Metrics.register(factory.createMetricName(name),
+ gauge,
+ aliasFactory.createMetricName(name),
+ factory.createMetricName(deprecated),
+ aliasFactory.createMetricName(deprecated));
+
+ if (register(name, name, deprecated, cfGauge))
Review comment:
shouldn't this also check that `globalGauge` isn't null?
##########
File path: src/java/org/apache/cassandra/metrics/ClientMetrics.java
##########
@@ -136,16 +140,19 @@ private int countConnectedClients()
for (Server server : servers)
for (ClientStat stat : server.recentClientStats())
- stats.add(new HashMap(stat.asMap())); // asMap returns guava, so need to convert to java for jmx
+ stats.add(new HashMap<>(stat.asMap())); // asMap returns guava, so need to convert to java for jmx
stats.sort(Comparator.comparing(map -> map.get(ClientStat.PROTOCOL_VERSION)));
return stats;
}
- private <T> Gauge<T> registerGauge(String name, Gauge<T> gauge)
+ private <T> Gauge<T> registerGauge(String name, String deprecated, Gauge<T> gauge)
Review comment:
would be nice to have an overload which defaults `deprecated` to null, mostly to cleanup `PausedConnections`
##########
File path: src/java/org/apache/cassandra/metrics/TableMetrics.java
##########
@@ -300,42 +299,26 @@ public Double getValue()
});
public static final Gauge<Long> globalBytesRepaired = Metrics.register(globalFactory.createMetricName("BytesRepaired"),
- new Gauge<Long>()
- {
- public Long getValue()
- {
- return totalNonSystemTablesSize(SSTableReader::isRepaired).left;
- }
- });
+ () -> totalNonSystemTablesSize(SSTableReader::isRepaired).left);
- public static final Gauge<Long> globalBytesUnrepaired = Metrics.register(globalFactory.createMetricName("BytesUnrepaired"),
- new Gauge<Long>()
- {
- public Long getValue()
- {
- return totalNonSystemTablesSize(s -> !s.isRepaired() && !s.isPendingRepair()).left;
- }
- });
+ public static final Gauge<Long> globalBytesUnrepaired =
+ Metrics.register(globalFactory.createMetricName("BytesUnrepaired"),
+ () -> totalNonSystemTablesSize(s -> !s.isRepaired() && !s.isPendingRepair()).left);
- public static final Gauge<Long> globalBytesPendingRepair = Metrics.register(globalFactory.createMetricName("BytesPendingRepair"),
- new Gauge<Long>()
- {
- public Long getValue()
- {
- return totalNonSystemTablesSize(SSTableReader::isPendingRepair).left;
- }
- });
+ public static final Gauge<Long> globalBytesPendingRepair =
+ Metrics.register(globalFactory.createMetricName("BytesPendingRepair"),
+ () -> totalNonSystemTablesSize(SSTableReader::isPendingRepair).left);
public final Meter readRepairRequests;
public final Meter shortReadProtectionRequests;
public final Meter replicaFilteringProtectionRequests;
/**
- * This histogram records the maximum number of rows {@link org.apache.cassandra.service.ReplicaFilteringProtection}
+ * This histogram records the maximum number of rows {@code ReplicaFilteringProtection}
Review comment:
why move from link? with link I get an anchor that will open the class if I click on it
##########
File path: src/java/org/apache/cassandra/metrics/TableMetrics.java
##########
@@ -1027,6 +953,35 @@ public Long getValue()
return cfGauge;
}
+ /**
+ * Same as {@link #createTableGauge(String, Gauge, Gauge)} but accepts a deprecated
+ * name for a table {@code Gauge}. Prefer that method when deprecation is not necessary.
+ *
+ * @param name the name of the metric registered with the "Table" type
+ * @param deprecated the deprecated name for the metric registered with the "Table" type
+ */
+ protected <G,T> Gauge<T> createTableGaugeWithDeprecation(String name, String deprecated, Gauge<T> gauge, Gauge<G> globalGauge)
Review comment:
this looks mostly the same as `org.apache.cassandra.metrics.TableMetrics#createTableGauge(java.lang.String, java.lang.String, com.codahale.metrics.Gauge<T>, com.codahale.metrics.Gauge<G>)`
##########
File path: src/java/org/apache/cassandra/metrics/TableMetrics.java
##########
@@ -443,35 +426,24 @@ public String toString(String value)
samplers.put(SamplerType.CAS_CONTENTIONS, topCasPartitionContention);
samplers.put(SamplerType.LOCAL_READ_TIME, topLocalReadQueryTime);
- memtableColumnsCount = createTableGauge("MemtableColumnsCount", new Gauge<Long>()
- {
- public Long getValue()
- {
- return cfs.getTracker().getView().getCurrentMemtable().getOperations();
- }
- });
- memtableOnHeapSize = createTableGauge("MemtableOnHeapSize", new Gauge<Long>()
- {
- public Long getValue()
- {
- return cfs.getTracker().getView().getCurrentMemtable().getAllocator().onHeap().owns();
- }
- });
- memtableOffHeapSize = createTableGauge("MemtableOffHeapSize", new Gauge<Long>()
- {
- public Long getValue()
- {
- return cfs.getTracker().getView().getCurrentMemtable().getAllocator().offHeap().owns();
- }
- });
- memtableLiveDataSize = createTableGauge("MemtableLiveDataSize", new Gauge<Long>()
- {
- public Long getValue()
- {
- return cfs.getTracker().getView().getCurrentMemtable().getLiveDataSize();
- }
- });
- allMemtablesOnHeapSize = createTableGauge("AllMemtablesHeapSize", new Gauge<Long>()
+ memtableColumnsCount = createTableGauge("MemtableColumnsCount",
+ () -> cfs.getTracker().getView().getCurrentMemtable().getOperations());
+
+ // MemtableOnHeapSize naming deprecated in 4.0
+ memtableOnHeapDataSize = createTableGaugeWithDeprecation("MemtableOnHeapDataSize", "MemtableOnHeapSize",
+ () -> cfs.getTracker().getView().getCurrentMemtable().getAllocator().onHeap().owns(),
+ new GlobalTableGauge("MemtableOnHeapDataSize"));
+
+ // MemtableOffHeapSize naming deprecated in 4.0
+ memtableOffHeapDataSize = createTableGaugeWithDeprecation("MemtableOffHeapDataSize", "MemtableOffHeapSize",
+ () -> cfs.getTracker().getView().getCurrentMemtable().getAllocator().offHeap().owns(),
+ new GlobalTableGauge("MemtableOnHeapDataSize"));
+
+ memtableLiveDataSize = createTableGauge("MemtableLiveDataSize",
+ () -> cfs.getTracker().getView().getCurrentMemtable().getLiveDataSize());
+
+ // AllMemtablesHeapSize naming deprecated in 4.0
+ allMemtablesOnHeapDataSize = createTableGaugeWithDeprecation("AllMemtablesOnHeapDataSize", "AllMemtablesHeapSize", new Gauge<Long>()
Review comment:
since the rest went lambda, any reason to not go lambda here?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org