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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
