You are viewing a plain text version of this content. The canonical link for it is here.
Posted to pr@cassandra.apache.org by "Mmuzaf (via GitHub)" <gi...@apache.org> on 2024/04/19 18:09:06 UTC
[PR] CASSANDRA-19567 Reduce heap usage by metrics [cassandra]
Mmuzaf opened a new pull request, #3267:
URL: https://github.com/apache/cassandra/pull/3267
Thanks for sending a pull request! Here are some tips if you're new here:
* Ensure you have added or run the [appropriate tests](https://cassandra.apache.org/_/development/testing.html) for your PR.
* Be sure to keep the PR description updated to reflect all changes.
* Write your PR title to summarize what this PR proposes.
* If possible, provide a concise example to reproduce the issue for a faster review.
* Read our [contributor guidelines](https://cassandra.apache.org/_/development/index.html)
* If you're making a documentation change, see our [guide to documentation contribution](https://cassandra.apache.org/_/development/documentation.html)
Commit messages should follow the following format:
```
<One sentence description, usually Jira title or CHANGES.txt summary>
<Optional lengthier description (context on patch)>
patch by <Authors>; reviewed by <Reviewers> for CASSANDRA-#####
Co-authored-by: Name1 <email1>
Co-authored-by: Name2 <email2>
```
The [Cassandra Jira](https://issues.apache.org/jira/projects/CASSANDRA/issues/)
--
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.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
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
Re: [PR] CASSANDRA-19567 Reduce heap usage by metrics [cassandra]
Posted by "maedhroz (via GitHub)" <gi...@apache.org>.
maedhroz commented on code in PR #3267:
URL: https://github.com/apache/cassandra/pull/3267#discussion_r1579830376
##########
src/java/org/apache/cassandra/metrics/CassandraMetricsRegistry.java:
##########
@@ -444,30 +410,71 @@ void remove(ThreadPoolMetrics metrics)
public <T extends Metric> T register(MetricName name, T metric, MetricName... aliases)
{
- setAliases(ArrayUtils.addAll(new MetricName[]{name}, aliases));
T metricLoc = register(name, metric);
Stream.of(aliases).forEach(n -> register(n, metricLoc));
return metricLoc;
}
+ /**
+ * Removes all metrics that match the given predicate.
+ *
+ * @param resolver a function that resolves a short metric name from a full metric name,
+ * or @{code null} if the metric should not be removed
+ * @param factory a function that creates a metric name from a short metric name.
+ * @param onRemoved a callback that is called for each removed metric.
+ */
+ public void removeIfMatch(MetricNameResolver resolver,
+ Function<String, MetricName> factory,
+ Consumer<MetricName> onRemoved)
+ {
+ removeMatching((full, metric) -> {
+ String shortName = resolver.resolve(full);
+ if (shortName == null)
+ return false;
+
+ MetricName metricName = factory.apply(shortName);
+ boolean remove = metricName.getMetricName().equals(full);
+
+ if (remove)
+ {
+ unregisterMBean(metricName.getMBeanName(), MBeanWrapper.instance);
+ onRemoved.accept(metricName);
+ }
+ return remove;
+ });
+ }
+
+ /**
+ * Default implementation of the {@link MetricNameResolver} that resolves a short metric name from a full metric name,
+ * assuming that the full metric name doesn't contain dots. Returns {@code null} if the full metric name doesn't match
+ * the provided group and type.
+ *
+ * @param fullName full metric name
+ * @param group metric group
+ * @param type metric type
+ * @return short metric name or {@code null} if the full metric name doesn't match the group and type.
+ */
+ public static @Nullable String resolveShortMetricName(String fullName, String group, String type)
+ {
+ String prefix = name(group, type);
+ if (fullName.startsWith(prefix))
+ return fullName.substring(prefix.length() + 1, fullName.indexOf('.', prefix.length() + 1));
+
+ return null;
+ }
+
public void remove(MetricName name)
{
// Aliases are removed in onMetricRemoved by metrics listener.
Review Comment:
nit: This comment is no longer true?
--
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.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
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
Re: [PR] CASSANDRA-19567 Reduce heap usage by metrics [cassandra]
Posted by "Mmuzaf (via GitHub)" <gi...@apache.org>.
Mmuzaf commented on PR #3267:
URL: https://github.com/apache/cassandra/pull/3267#issuecomment-2087154655
Committed:
https://github.com/apache/cassandra/commit/8619010cdca8bce471754e4fbeb861f036535007
--
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.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
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
Re: [PR] CASSANDRA-19567 Reduce heap usage by metrics [cassandra]
Posted by "Mmuzaf (via GitHub)" <gi...@apache.org>.
Mmuzaf commented on code in PR #3267:
URL: https://github.com/apache/cassandra/pull/3267#discussion_r1582279651
##########
src/java/org/apache/cassandra/metrics/CassandraMetricsRegistry.java:
##########
@@ -444,30 +410,71 @@ void remove(ThreadPoolMetrics metrics)
public <T extends Metric> T register(MetricName name, T metric, MetricName... aliases)
{
- setAliases(ArrayUtils.addAll(new MetricName[]{name}, aliases));
T metricLoc = register(name, metric);
Stream.of(aliases).forEach(n -> register(n, metricLoc));
return metricLoc;
}
+ /**
+ * Removes all metrics that match the given predicate.
+ *
+ * @param resolver a function that resolves a short metric name from a full metric name,
+ * or @{code null} if the metric should not be removed
+ * @param factory a function that creates a metric name from a short metric name.
+ * @param onRemoved a callback that is called for each removed metric.
+ */
+ public void removeIfMatch(MetricNameResolver resolver,
+ Function<String, MetricName> factory,
+ Consumer<MetricName> onRemoved)
+ {
+ removeMatching((full, metric) -> {
+ String shortName = resolver.resolve(full);
+ if (shortName == null)
+ return false;
+
+ MetricName metricName = factory.apply(shortName);
+ boolean remove = metricName.getMetricName().equals(full);
+
+ if (remove)
+ {
+ unregisterMBean(metricName.getMBeanName(), MBeanWrapper.instance);
+ onRemoved.accept(metricName);
+ }
+ return remove;
+ });
+ }
+
+ /**
+ * Default implementation of the {@link MetricNameResolver} that resolves a short metric name from a full metric name,
+ * assuming that the full metric name doesn't contain dots. Returns {@code null} if the full metric name doesn't match
+ * the provided group and type.
+ *
+ * @param fullName full metric name
+ * @param group metric group
+ * @param type metric type
+ * @return short metric name or {@code null} if the full metric name doesn't match the group and type.
+ */
+ public static @Nullable String resolveShortMetricName(String fullName, String group, String type)
+ {
+ String prefix = name(group, type);
+ if (fullName.startsWith(prefix))
+ return fullName.substring(prefix.length() + 1, fullName.indexOf('.', prefix.length() + 1));
+
+ return null;
+ }
+
public void remove(MetricName name)
{
// Aliases are removed in onMetricRemoved by metrics listener.
- remove(name.getMetricName());
+ boolean success = remove(name.getMetricName());
+ if (success)
Review Comment:
`!success` should never happen in general, but I've noticed that because of the hierarchical structure of some metrics and the lifecycle we have, we can try to release a metric more than once.
The `mBeanWrapper.unregisterMBean(name, MBeanWrapper.OnException.IGNORE);` indicates that some of the exceptions about missing requested metrics are simply ignored due to the fact that the requested metric is already released.
So I'd keep it as is for safety, but we can change it and see how many tests will fail.
--
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.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
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
Re: [PR] CASSANDRA-19567 Reduce heap usage by metrics [cassandra]
Posted by "maedhroz (via GitHub)" <gi...@apache.org>.
maedhroz commented on code in PR #3267:
URL: https://github.com/apache/cassandra/pull/3267#discussion_r1579775946
##########
src/java/org/apache/cassandra/db/memtable/Memtable.java:
##########
@@ -144,7 +143,7 @@ default boolean streamFromMemtable()
* Memtable metrics lifecycle matches table lifecycle. It is the table that owns the metrics and
* decides when to release them.
*/
- default TableMetrics.ReleasableMetric createMemtableMetrics(TableMetadataRef metadataRef)
+ default TrieMemtableMetricsView createMemtableMetrics(TableMetadataRef metadataRef)
Review Comment:
It seems like we'd want to keep something like the `ReleasableMetric` interface to keep details about particular implementations out of the `Memtable` interface. What am I missing?
--
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.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
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
Re: [PR] CASSANDRA-19567 Reduce heap usage by metrics [cassandra]
Posted by "Mmuzaf (via GitHub)" <gi...@apache.org>.
Mmuzaf commented on code in PR #3267:
URL: https://github.com/apache/cassandra/pull/3267#discussion_r1582196121
##########
src/java/org/apache/cassandra/index/sai/metrics/AbstractMetrics.java:
##########
@@ -57,28 +55,28 @@ public abstract class AbstractMetrics
public void release()
{
- tracked.forEach(Metrics::remove);
- tracked.clear();
+ Metrics.removeIfMatch(fullName -> resolveShortMetricName(fullName, DefaultNameFactory.GROUP_NAME, TYPE),
+ this::createMetricName, m -> {});
}
- protected CassandraMetricsRegistry.MetricName createMetricName(String name)
+ protected String metricScope()
{
- return createMetricName(name, scope);
+ return scope;
}
- protected CassandraMetricsRegistry.MetricName createMetricName(String name, String scope)
+ protected CassandraMetricsRegistry.MetricName createMetricName(String name)
{
- String metricScope = keyspace + '.' + table;
- if (index != null)
- {
- metricScope += '.' + index;
- }
- metricScope += '.' + scope + '.' + name;
+ return createMetricName(name, metricScope());
+ }
- CassandraMetricsRegistry.MetricName metricName = new CassandraMetricsRegistry.MetricName(DefaultNameFactory.GROUP_NAME,
- TYPE, name, metricScope, createMBeanName(name, scope));
- tracked.add(metricName.getMetricName());
- return metricName;
+ private CassandraMetricsRegistry.MetricName createMetricName(String name, String scope)
+ {
+ assert name.indexOf('.') == -1 : String.format("Metric name '%s' should not contain '.'", name);
+ return new CassandraMetricsRegistry.MetricName(DefaultNameFactory.GROUP_NAME,
+ TYPE,
+ name,
+ MetricRegistry.name(keyspace, table, index, scope, name),
Review Comment:
Some of the metrics are created with `scope = null`, so the `resolveShortMetricName` method won't work in this case. This is an implementation bug. Fixed.
##########
src/java/org/apache/cassandra/metrics/CassandraMetricsRegistry.java:
##########
@@ -444,30 +410,71 @@ void remove(ThreadPoolMetrics metrics)
public <T extends Metric> T register(MetricName name, T metric, MetricName... aliases)
{
- setAliases(ArrayUtils.addAll(new MetricName[]{name}, aliases));
T metricLoc = register(name, metric);
Stream.of(aliases).forEach(n -> register(n, metricLoc));
return metricLoc;
}
+ /**
+ * Removes all metrics that match the given predicate.
+ *
+ * @param resolver a function that resolves a short metric name from a full metric name,
+ * or @{code null} if the metric should not be removed
+ * @param factory a function that creates a metric name from a short metric name.
+ * @param onRemoved a callback that is called for each removed metric.
+ */
+ public void removeIfMatch(MetricNameResolver resolver,
+ Function<String, MetricName> factory,
+ Consumer<MetricName> onRemoved)
+ {
+ removeMatching((full, metric) -> {
+ String shortName = resolver.resolve(full);
+ if (shortName == null)
+ return false;
+
+ MetricName metricName = factory.apply(shortName);
+ boolean remove = metricName.getMetricName().equals(full);
+
+ if (remove)
+ {
+ unregisterMBean(metricName.getMBeanName(), MBeanWrapper.instance);
+ onRemoved.accept(metricName);
+ }
+ return remove;
+ });
+ }
+
+ /**
+ * Default implementation of the {@link MetricNameResolver} that resolves a short metric name from a full metric name,
+ * assuming that the full metric name doesn't contain dots. Returns {@code null} if the full metric name doesn't match
+ * the provided group and type.
+ *
+ * @param fullName full metric name
+ * @param group metric group
+ * @param type metric type
+ * @return short metric name or {@code null} if the full metric name doesn't match the group and type.
+ */
+ public static @Nullable String resolveShortMetricName(String fullName, String group, String type)
+ {
+ String prefix = name(group, type);
+ if (fullName.startsWith(prefix))
+ return fullName.substring(prefix.length() + 1, fullName.indexOf('.', prefix.length() + 1));
+
+ return null;
+ }
+
public void remove(MetricName name)
{
// Aliases are removed in onMetricRemoved by metrics listener.
Review Comment:
Comment removed.
--
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.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
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
Re: [PR] CASSANDRA-19567 Reduce heap usage by metrics [cassandra]
Posted by "maedhroz (via GitHub)" <gi...@apache.org>.
maedhroz commented on code in PR #3267:
URL: https://github.com/apache/cassandra/pull/3267#discussion_r1579786449
##########
src/java/org/apache/cassandra/db/ColumnFamilyStore.java:
##########
@@ -508,12 +510,15 @@ public ColumnFamilyStore(Keyspace keyspace,
// Create Memtable and its metrics object only on online
Memtable initialMemtable = null;
- TableMetrics.ReleasableMetric memtableMetrics = null;
if (DatabaseDescriptor.isDaemonInitialized())
{
initialMemtable = createMemtable(new AtomicReference<>(CommitLog.instance.getCurrentPosition()));
- memtableMetrics = memtableFactory.createMemtableMetrics(metadata);
+ this.memtableMetrics = memtableFactory.createMemtableMetrics(metadata);
}
+ else
+ this.memtableMetrics = null;
+
+ metric = new TableMetrics(this);
Review Comment:
This appears to reintroduce [CASSANDRA-16228](https://issues.apache.org/jira/browse/CASSANDRA-16228)
--
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.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
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
Re: [PR] CASSANDRA-19567 Reduce heap usage by metrics [cassandra]
Posted by "maedhroz (via GitHub)" <gi...@apache.org>.
maedhroz commented on code in PR #3267:
URL: https://github.com/apache/cassandra/pull/3267#discussion_r1579785585
##########
src/java/org/apache/cassandra/db/ColumnFamilyStore.java:
##########
@@ -290,6 +291,7 @@ public enum FlushReason
private volatile boolean valid = true;
private volatile Memtable.Factory memtableFactory;
+ private final TrieMemtableMetricsView memtableMetrics;
Review Comment:
see https://github.com/apache/cassandra/pull/3267/files#r1579775946
--
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.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
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
Re: [PR] CASSANDRA-19567 Reduce heap usage by metrics [cassandra]
Posted by "maedhroz (via GitHub)" <gi...@apache.org>.
maedhroz commented on code in PR #3267:
URL: https://github.com/apache/cassandra/pull/3267#discussion_r1585189634
##########
src/java/org/apache/cassandra/index/sai/metrics/ColumnQueryMetrics.java:
##########
@@ -97,6 +104,14 @@ public BalancedTreeIndexMetrics(IndexIdentifier indexIdentifier)
postingsListener = new PostingListEventsMetrics(postingDecodes);
}
+ @Override
+ public void release()
+ {
+ super.release();
+ Metrics.remove(createMetricName("NumPostings", BALANCED_TREE_POSTINGS_TYPE));
+ Metrics.remove(createMetricName("PostingDecodes", BALANCED_TREE_POSTINGS_TYPE));
Review Comment:
nit: We could make constants out of "PostingDecodes" and "NumPostings" now, I guess, since we use them multiple places, but not a big deal either way.
--
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.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
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
Re: [PR] CASSANDRA-19567 Reduce heap usage by metrics [cassandra]
Posted by "maedhroz (via GitHub)" <gi...@apache.org>.
maedhroz commented on code in PR #3267:
URL: https://github.com/apache/cassandra/pull/3267#discussion_r1585186787
##########
src/java/org/apache/cassandra/db/ColumnFamilyStore.java:
##########
@@ -545,7 +543,9 @@ public ColumnFamilyStore(Keyspace keyspace,
indexManager.addIndex(info, true);
}
- metric = new TableMetrics(this, memtableMetrics);
+ // See CASSANDRA-16228. We need to ensure that metrics are exposed after the CFS is initialized,
+ // so the order of the following line is important and should not be moved.
+ metric = new TableMetrics(this);
Review Comment:
Thanks for adding the comment ;)
--
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.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
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
Re: [PR] CASSANDRA-19567 Reduce heap usage by metrics [cassandra]
Posted by "maedhroz (via GitHub)" <gi...@apache.org>.
maedhroz commented on code in PR #3267:
URL: https://github.com/apache/cassandra/pull/3267#discussion_r1585200679
##########
src/java/org/apache/cassandra/metrics/CassandraMetricsRegistry.java:
##########
@@ -93,34 +89,14 @@ public class CassandraMetricsRegistry extends MetricRegistry
{
public static final UnaryOperator<String> METRICS_GROUP_POSTFIX = name -> name + "_group";
- /**
- * A map of metric name constructed by {@link com.codahale.metrics.MetricRegistry#name(String, String...)} and
- * its full name in the way how it is represented in JMX. The map is used by {@link CassandraJmxMetricsExporter}
- * to export metrics to JMX.
- */
- private static final ConcurrentMap<String, Deque<MetricName>> ALIASES = new ConcurrentHashMap<>();
Review Comment:
@krummas FYI, this should take care of [CASSANDRA-19571](https://issues.apache.org/jira/browse/CASSANDRA-19571)
--
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.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
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
Re: [PR] CASSANDRA-19567 Reduce heap usage by metrics [cassandra]
Posted by "Mmuzaf (via GitHub)" <gi...@apache.org>.
Mmuzaf closed pull request #3267: CASSANDRA-19567 Reduce heap usage by metrics
URL: https://github.com/apache/cassandra/pull/3267
--
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.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
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
Re: [PR] CASSANDRA-19567 Reduce heap usage by metrics [cassandra]
Posted by "Mmuzaf (via GitHub)" <gi...@apache.org>.
Mmuzaf commented on code in PR #3267:
URL: https://github.com/apache/cassandra/pull/3267#discussion_r1582195614
##########
src/java/org/apache/cassandra/db/ColumnFamilyStore.java:
##########
@@ -508,12 +510,15 @@ public ColumnFamilyStore(Keyspace keyspace,
// Create Memtable and its metrics object only on online
Memtable initialMemtable = null;
- TableMetrics.ReleasableMetric memtableMetrics = null;
if (DatabaseDescriptor.isDaemonInitialized())
{
initialMemtable = createMemtable(new AtomicReference<>(CommitLog.instance.getCurrentPosition()));
- memtableMetrics = memtableFactory.createMemtableMetrics(metadata);
+ this.memtableMetrics = memtableFactory.createMemtableMetrics(metadata);
}
+ else
+ this.memtableMetrics = null;
+
+ metric = new TableMetrics(this);
Review Comment:
Yes, I've added an extra comment for that. The initialization of metrics is critical, depending on which line they are created from.
--
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.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
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
Re: [PR] CASSANDRA-19567 Reduce heap usage by metrics [cassandra]
Posted by "maedhroz (via GitHub)" <gi...@apache.org>.
maedhroz commented on code in PR #3267:
URL: https://github.com/apache/cassandra/pull/3267#discussion_r1585185112
##########
src/java/org/apache/cassandra/db/ColumnFamilyStore.java:
##########
@@ -320,6 +320,7 @@ public enum FlushReason
private final Directories directories;
public final TableMetrics metric;
+ private final Runnable metricsReleaser;
Review Comment:
nit: `memtableMetricsReleaser`?
--
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.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
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
Re: [PR] CASSANDRA-19567 Reduce heap usage by metrics [cassandra]
Posted by "Mmuzaf (via GitHub)" <gi...@apache.org>.
Mmuzaf commented on code in PR #3267:
URL: https://github.com/apache/cassandra/pull/3267#discussion_r1582277589
##########
src/java/org/apache/cassandra/db/memtable/Memtable.java:
##########
@@ -144,7 +143,7 @@ default boolean streamFromMemtable()
* Memtable metrics lifecycle matches table lifecycle. It is the table that owns the metrics and
* decides when to release them.
*/
- default TableMetrics.ReleasableMetric createMemtableMetrics(TableMetadataRef metadataRef)
+ default TrieMemtableMetricsView createMemtableMetrics(TableMetadataRef metadataRef)
Review Comment:
I've looked at it again and I think we can remove the `createMemtableMetrics` method, as it doesn't directly instantiate metrics for memtables and the metric handling is a bit confusing so far.
The metric class instance created by the `createMemtableMetrics` method differs from the metrics created for the memtables themselves via a constructor (see [new TrieMemtable(...)](https://github.com/apache/cassandra/blob/trunk/src/java/org/apache/cassandra/db/memtable/TrieMemtable.java#L126)). So far they only match and released just because the metric names are exactly the same. All `TrieMemtableMetricsView` share the same `Metric` instances, but if we add a memtable-specific metric it will immediately lead to a memory leak as this metric won't be released properly.
I suggest explicitly releasing all metrics assigned to the memtables (e.g. by the `TrieMemtableMetricsView`), even if it ends up being released only once. I've pushed the changes into the PR. It seems to be much cleaner now. WDYT?
--
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.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
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
Re: [PR] CASSANDRA-19567 Reduce heap usage by metrics [cassandra]
Posted by "maedhroz (via GitHub)" <gi...@apache.org>.
maedhroz commented on code in PR #3267:
URL: https://github.com/apache/cassandra/pull/3267#discussion_r1579833686
##########
src/java/org/apache/cassandra/metrics/CassandraMetricsRegistry.java:
##########
@@ -444,30 +410,71 @@ void remove(ThreadPoolMetrics metrics)
public <T extends Metric> T register(MetricName name, T metric, MetricName... aliases)
{
- setAliases(ArrayUtils.addAll(new MetricName[]{name}, aliases));
T metricLoc = register(name, metric);
Stream.of(aliases).forEach(n -> register(n, metricLoc));
return metricLoc;
}
+ /**
+ * Removes all metrics that match the given predicate.
+ *
+ * @param resolver a function that resolves a short metric name from a full metric name,
+ * or @{code null} if the metric should not be removed
+ * @param factory a function that creates a metric name from a short metric name.
+ * @param onRemoved a callback that is called for each removed metric.
+ */
+ public void removeIfMatch(MetricNameResolver resolver,
+ Function<String, MetricName> factory,
+ Consumer<MetricName> onRemoved)
+ {
+ removeMatching((full, metric) -> {
+ String shortName = resolver.resolve(full);
+ if (shortName == null)
+ return false;
+
+ MetricName metricName = factory.apply(shortName);
+ boolean remove = metricName.getMetricName().equals(full);
+
+ if (remove)
+ {
+ unregisterMBean(metricName.getMBeanName(), MBeanWrapper.instance);
+ onRemoved.accept(metricName);
+ }
+ return remove;
+ });
+ }
+
+ /**
+ * Default implementation of the {@link MetricNameResolver} that resolves a short metric name from a full metric name,
+ * assuming that the full metric name doesn't contain dots. Returns {@code null} if the full metric name doesn't match
+ * the provided group and type.
+ *
+ * @param fullName full metric name
+ * @param group metric group
+ * @param type metric type
+ * @return short metric name or {@code null} if the full metric name doesn't match the group and type.
+ */
+ public static @Nullable String resolveShortMetricName(String fullName, String group, String type)
+ {
+ String prefix = name(group, type);
+ if (fullName.startsWith(prefix))
+ return fullName.substring(prefix.length() + 1, fullName.indexOf('.', prefix.length() + 1));
+
+ return null;
+ }
+
public void remove(MetricName name)
{
// Aliases are removed in onMetricRemoved by metrics listener.
- remove(name.getMetricName());
+ boolean success = remove(name.getMetricName());
+ if (success)
Review Comment:
nit: Should we ever call `remove()` here and have `!success`? It seems like that would be a programmer error...and something we should perhaps make more visible w/ logging, `assert`, etc.?
--
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.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
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
Re: [PR] CASSANDRA-19567 Reduce heap usage by metrics [cassandra]
Posted by "maedhroz (via GitHub)" <gi...@apache.org>.
maedhroz commented on code in PR #3267:
URL: https://github.com/apache/cassandra/pull/3267#discussion_r1579832099
##########
src/java/org/apache/cassandra/metrics/CassandraMetricsRegistry.java:
##########
@@ -444,30 +410,71 @@ void remove(ThreadPoolMetrics metrics)
public <T extends Metric> T register(MetricName name, T metric, MetricName... aliases)
{
- setAliases(ArrayUtils.addAll(new MetricName[]{name}, aliases));
T metricLoc = register(name, metric);
Stream.of(aliases).forEach(n -> register(n, metricLoc));
return metricLoc;
}
+ /**
+ * Removes all metrics that match the given predicate.
+ *
+ * @param resolver a function that resolves a short metric name from a full metric name,
+ * or @{code null} if the metric should not be removed
+ * @param factory a function that creates a metric name from a short metric name.
+ * @param onRemoved a callback that is called for each removed metric.
+ */
+ public void removeIfMatch(MetricNameResolver resolver,
+ Function<String, MetricName> factory,
+ Consumer<MetricName> onRemoved)
+ {
+ removeMatching((full, metric) -> {
+ String shortName = resolver.resolve(full);
+ if (shortName == null)
+ return false;
+
+ MetricName metricName = factory.apply(shortName);
+ boolean remove = metricName.getMetricName().equals(full);
+
+ if (remove)
+ {
+ unregisterMBean(metricName.getMBeanName(), MBeanWrapper.instance);
+ onRemoved.accept(metricName);
+ }
+ return remove;
+ });
+ }
+
+ /**
+ * Default implementation of the {@link MetricNameResolver} that resolves a short metric name from a full metric name,
+ * assuming that the full metric name doesn't contain dots. Returns {@code null} if the full metric name doesn't match
+ * the provided group and type.
+ *
+ * @param fullName full metric name
+ * @param group metric group
+ * @param type metric type
+ * @return short metric name or {@code null} if the full metric name doesn't match the group and type.
+ */
+ public static @Nullable String resolveShortMetricName(String fullName, String group, String type)
+ {
+ String prefix = name(group, type);
+ if (fullName.startsWith(prefix))
+ return fullName.substring(prefix.length() + 1, fullName.indexOf('.', prefix.length() + 1));
+
+ return null;
+ }
+
public void remove(MetricName name)
{
// Aliases are removed in onMetricRemoved by metrics listener.
Review Comment:
The way this works does make more sense now, though. The table metrics were the only place we cared about the aliases, and they handle removal specifically now.
--
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.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
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
Re: [PR] CASSANDRA-19567 Reduce heap usage by metrics [cassandra]
Posted by "maedhroz (via GitHub)" <gi...@apache.org>.
maedhroz commented on code in PR #3267:
URL: https://github.com/apache/cassandra/pull/3267#discussion_r1579811401
##########
src/java/org/apache/cassandra/index/sai/metrics/AbstractMetrics.java:
##########
@@ -57,28 +55,28 @@ public abstract class AbstractMetrics
public void release()
{
- tracked.forEach(Metrics::remove);
- tracked.clear();
+ Metrics.removeIfMatch(fullName -> resolveShortMetricName(fullName, DefaultNameFactory.GROUP_NAME, TYPE),
+ this::createMetricName, m -> {});
}
- protected CassandraMetricsRegistry.MetricName createMetricName(String name)
+ protected String metricScope()
{
- return createMetricName(name, scope);
+ return scope;
}
- protected CassandraMetricsRegistry.MetricName createMetricName(String name, String scope)
+ protected CassandraMetricsRegistry.MetricName createMetricName(String name)
{
- String metricScope = keyspace + '.' + table;
- if (index != null)
- {
- metricScope += '.' + index;
- }
- metricScope += '.' + scope + '.' + name;
+ return createMetricName(name, metricScope());
+ }
- CassandraMetricsRegistry.MetricName metricName = new CassandraMetricsRegistry.MetricName(DefaultNameFactory.GROUP_NAME,
- TYPE, name, metricScope, createMBeanName(name, scope));
- tracked.add(metricName.getMetricName());
- return metricName;
+ private CassandraMetricsRegistry.MetricName createMetricName(String name, String scope)
+ {
+ assert name.indexOf('.') == -1 : String.format("Metric name '%s' should not contain '.'", name);
+ return new CassandraMetricsRegistry.MetricName(DefaultNameFactory.GROUP_NAME,
+ TYPE,
+ name,
+ MetricRegistry.name(keyspace, table, index, scope, name),
Review Comment:
It feels like something around this change is causing problems w/ `index` handling that pop up in `GuardrailNonPartitionRestrictedQueryTest`, `IndexAvailabilityTest`, and a few other places.
--
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.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
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
Re: [PR] CASSANDRA-19567 Reduce heap usage by metrics [cassandra]
Posted by "maedhroz (via GitHub)" <gi...@apache.org>.
maedhroz commented on code in PR #3267:
URL: https://github.com/apache/cassandra/pull/3267#discussion_r1579783384
##########
src/java/org/apache/cassandra/db/memtable/Memtable.java:
##########
@@ -144,7 +143,7 @@ default boolean streamFromMemtable()
* Memtable metrics lifecycle matches table lifecycle. It is the table that owns the metrics and
* decides when to release them.
*/
- default TableMetrics.ReleasableMetric createMemtableMetrics(TableMetadataRef metadataRef)
+ default TrieMemtableMetricsView createMemtableMetrics(TableMetadataRef metadataRef)
Review Comment:
Perhaps the idea is trying to avoid creating a new lambda in `TrieMemtable`, but we could just have `TrieMemtableMetricsView` implement `ReleasableMetric` right?
--
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.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
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