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:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
