From 1065ee9cf4c6c439413ff2797ff3660fe7e36cd1 Mon Sep 17 00:00:00 2001 From: janehe Date: Fri, 12 Apr 2024 13:20:33 -0700 Subject: [PATCH] patch by Jane He; reviewed by Alexandre Dutra and Bret McGuire for CASSANDRA-19457 --- .../core/metrics/AbstractMetricUpdater.java | 2 -- .../core/metrics/DropwizardMetricUpdater.java | 2 +- .../internal/core/metrics/MetricUpdater.java | 2 ++ .../core/metrics/NoopNodeMetricUpdater.java | 5 +++++ .../core/metrics/NoopSessionMetricUpdater.java | 3 +++ .../internal/core/session/DefaultSession.java | 15 +++++++++++++++ .../driver/core/metrics/DropwizardMetricsIT.java | 6 ++++++ .../oss/driver/core/metrics/MetricsITBase.java | 10 ++++++++-- .../metrics/micrometer/MicrometerMetricsIT.java | 6 ++++++ .../microprofile/MicroProfileMetricsIT.java | 6 ++++++ .../micrometer/MicrometerMetricUpdater.java | 2 +- .../microprofile/MicroProfileMetricUpdater.java | 2 +- 12 files changed, 54 insertions(+), 7 deletions(-) diff --git a/core/src/main/java/com/datastax/oss/driver/internal/core/metrics/AbstractMetricUpdater.java b/core/src/main/java/com/datastax/oss/driver/internal/core/metrics/AbstractMetricUpdater.java index fcfe56b605e..5e2392a2e7f 100644 --- a/core/src/main/java/com/datastax/oss/driver/internal/core/metrics/AbstractMetricUpdater.java +++ b/core/src/main/java/com/datastax/oss/driver/internal/core/metrics/AbstractMetricUpdater.java @@ -180,6 +180,4 @@ protected Timeout newTimeout() { expireAfter.toNanos(), TimeUnit.NANOSECONDS); } - - protected abstract void clearMetrics(); } diff --git a/core/src/main/java/com/datastax/oss/driver/internal/core/metrics/DropwizardMetricUpdater.java b/core/src/main/java/com/datastax/oss/driver/internal/core/metrics/DropwizardMetricUpdater.java index 8590917be21..9377fb3a17e 100644 --- a/core/src/main/java/com/datastax/oss/driver/internal/core/metrics/DropwizardMetricUpdater.java +++ b/core/src/main/java/com/datastax/oss/driver/internal/core/metrics/DropwizardMetricUpdater.java @@ -91,7 +91,7 @@ public void updateTimer( } @Override - protected void clearMetrics() { + public void clearMetrics() { for (MetricT metric : metrics.keySet()) { MetricId id = getMetricId(metric); registry.remove(id.getName()); diff --git a/core/src/main/java/com/datastax/oss/driver/internal/core/metrics/MetricUpdater.java b/core/src/main/java/com/datastax/oss/driver/internal/core/metrics/MetricUpdater.java index c4b432f3c50..c07d1b136af 100644 --- a/core/src/main/java/com/datastax/oss/driver/internal/core/metrics/MetricUpdater.java +++ b/core/src/main/java/com/datastax/oss/driver/internal/core/metrics/MetricUpdater.java @@ -46,4 +46,6 @@ default void markMeter(MetricT metric, @Nullable String profileName) { void updateTimer(MetricT metric, @Nullable String profileName, long duration, TimeUnit unit); boolean isEnabled(MetricT metric, @Nullable String profileName); + + void clearMetrics(); } diff --git a/core/src/main/java/com/datastax/oss/driver/internal/core/metrics/NoopNodeMetricUpdater.java b/core/src/main/java/com/datastax/oss/driver/internal/core/metrics/NoopNodeMetricUpdater.java index 45f0797c7b5..8d216990331 100644 --- a/core/src/main/java/com/datastax/oss/driver/internal/core/metrics/NoopNodeMetricUpdater.java +++ b/core/src/main/java/com/datastax/oss/driver/internal/core/metrics/NoopNodeMetricUpdater.java @@ -53,4 +53,9 @@ public boolean isEnabled(NodeMetric metric, String profileName) { // since methods don't do anything, return false return false; } + + @Override + public void clearMetrics() { + // nothing to do + } } diff --git a/core/src/main/java/com/datastax/oss/driver/internal/core/metrics/NoopSessionMetricUpdater.java b/core/src/main/java/com/datastax/oss/driver/internal/core/metrics/NoopSessionMetricUpdater.java index 1666261590c..7099a8ddcac 100644 --- a/core/src/main/java/com/datastax/oss/driver/internal/core/metrics/NoopSessionMetricUpdater.java +++ b/core/src/main/java/com/datastax/oss/driver/internal/core/metrics/NoopSessionMetricUpdater.java @@ -53,4 +53,7 @@ public boolean isEnabled(SessionMetric metric, String profileName) { // since methods don't do anything, return false return false; } + + @Override + public void clearMetrics() {} } diff --git a/core/src/main/java/com/datastax/oss/driver/internal/core/session/DefaultSession.java b/core/src/main/java/com/datastax/oss/driver/internal/core/session/DefaultSession.java index af9dc183f7e..cb1271c9cba 100644 --- a/core/src/main/java/com/datastax/oss/driver/internal/core/session/DefaultSession.java +++ b/core/src/main/java/com/datastax/oss/driver/internal/core/session/DefaultSession.java @@ -34,6 +34,7 @@ import com.datastax.oss.driver.internal.core.channel.DriverChannel; import com.datastax.oss.driver.internal.core.context.InternalDriverContext; import com.datastax.oss.driver.internal.core.context.LifecycleListener; +import com.datastax.oss.driver.internal.core.metadata.DefaultNode; import com.datastax.oss.driver.internal.core.metadata.MetadataManager; import com.datastax.oss.driver.internal.core.metadata.MetadataManager.RefreshSchemaResult; import com.datastax.oss.driver.internal.core.metadata.NodeStateEvent; @@ -546,6 +547,13 @@ private void close() { closePolicies(); + // clear metrics to prevent memory leak + for (Node n : metadataManager.getMetadata().getNodes().values()) { + ((DefaultNode) n).getMetricUpdater().clearMetrics(); + } + + DefaultSession.this.metricUpdater.clearMetrics(); + List> childrenCloseStages = new ArrayList<>(); for (AsyncAutoCloseable closeable : internalComponentsToClose()) { childrenCloseStages.add(closeable.closeAsync()); @@ -565,6 +573,13 @@ private void forceClose() { logPrefix, (closeWasCalled ? "" : "not ")); + // clear metrics to prevent memory leak + for (Node n : metadataManager.getMetadata().getNodes().values()) { + ((DefaultNode) n).getMetricUpdater().clearMetrics(); + } + + DefaultSession.this.metricUpdater.clearMetrics(); + if (closeWasCalled) { // onChildrenClosed has already been scheduled for (AsyncAutoCloseable closeable : internalComponentsToClose()) { diff --git a/integration-tests/src/test/java/com/datastax/oss/driver/core/metrics/DropwizardMetricsIT.java b/integration-tests/src/test/java/com/datastax/oss/driver/core/metrics/DropwizardMetricsIT.java index 6cbe443f2a6..e0184516e21 100644 --- a/integration-tests/src/test/java/com/datastax/oss/driver/core/metrics/DropwizardMetricsIT.java +++ b/integration-tests/src/test/java/com/datastax/oss/driver/core/metrics/DropwizardMetricsIT.java @@ -198,6 +198,12 @@ protected void assertNodeMetricsNotEvicted(CqlSession session, Node node) { } } + @Override + protected void assertMetricsNotPresent(Object registry) { + MetricRegistry dropwizardRegistry = (MetricRegistry) registry; + assertThat(dropwizardRegistry.getMetrics()).isEmpty(); + } + @Override protected void assertNodeMetricsEvicted(CqlSession session, Node node) { InternalDriverContext context = (InternalDriverContext) session.getContext(); diff --git a/integration-tests/src/test/java/com/datastax/oss/driver/core/metrics/MetricsITBase.java b/integration-tests/src/test/java/com/datastax/oss/driver/core/metrics/MetricsITBase.java index 7fac3f98f52..e6121217619 100644 --- a/integration-tests/src/test/java/com/datastax/oss/driver/core/metrics/MetricsITBase.java +++ b/integration-tests/src/test/java/com/datastax/oss/driver/core/metrics/MetricsITBase.java @@ -83,8 +83,10 @@ public void resetSimulacron() { @Test @UseDataProvider("descriptorsAndPrefixes") - public void should_expose_metrics_if_enabled(Class metricIdGenerator, String prefix) { + public void should_expose_metrics_if_enabled_and_clear_metrics_if_closed( + Class metricIdGenerator, String prefix) { + Object registry = newMetricRegistry(); Assume.assumeFalse( "Cannot use metric tags with Dropwizard", metricIdGenerator.getSimpleName().contains("Tagging") @@ -101,12 +103,14 @@ public void should_expose_metrics_if_enabled(Class metricIdGenerator, String CqlSession.builder() .addContactEndPoints(simulacron().getContactPoints()) .withConfigLoader(loader) - .withMetricRegistry(newMetricRegistry()) + .withMetricRegistry(registry) .build()) { session.prepare("irrelevant"); queryAllNodes(session); assertMetricsPresent(session); + } finally { + assertMetricsNotPresent(registry); } } @@ -262,4 +266,6 @@ private DefaultNode findNode(CqlSession session, int id) { return (DefaultNode) session.getMetadata().findNode(address1).orElseThrow(IllegalStateException::new); } + + protected abstract void assertMetricsNotPresent(Object registry); } diff --git a/integration-tests/src/test/java/com/datastax/oss/driver/metrics/micrometer/MicrometerMetricsIT.java b/integration-tests/src/test/java/com/datastax/oss/driver/metrics/micrometer/MicrometerMetricsIT.java index 5fe64719327..c38df1e2026 100644 --- a/integration-tests/src/test/java/com/datastax/oss/driver/metrics/micrometer/MicrometerMetricsIT.java +++ b/integration-tests/src/test/java/com/datastax/oss/driver/metrics/micrometer/MicrometerMetricsIT.java @@ -186,6 +186,12 @@ protected void assertNodeMetricsNotEvicted(CqlSession session, Node node) { } } + @Override + protected void assertMetricsNotPresent(Object registry) { + MeterRegistry micrometerRegistry = (MeterRegistry) registry; + assertThat(micrometerRegistry.getMeters()).isEmpty(); + } + @Override protected void assertNodeMetricsEvicted(CqlSession session, Node node) { InternalDriverContext context = (InternalDriverContext) session.getContext(); diff --git a/integration-tests/src/test/java/com/datastax/oss/driver/metrics/microprofile/MicroProfileMetricsIT.java b/integration-tests/src/test/java/com/datastax/oss/driver/metrics/microprofile/MicroProfileMetricsIT.java index 1294be3deae..aa04c058a49 100644 --- a/integration-tests/src/test/java/com/datastax/oss/driver/metrics/microprofile/MicroProfileMetricsIT.java +++ b/integration-tests/src/test/java/com/datastax/oss/driver/metrics/microprofile/MicroProfileMetricsIT.java @@ -188,6 +188,12 @@ protected void assertNodeMetricsNotEvicted(CqlSession session, Node node) { } } + @Override + protected void assertMetricsNotPresent(Object registry) { + MetricRegistry metricRegistry = (MetricRegistry) registry; + assertThat(metricRegistry.getMetrics()).isEmpty(); + } + @Override protected void assertNodeMetricsEvicted(CqlSession session, Node node) { InternalDriverContext context = (InternalDriverContext) session.getContext(); diff --git a/metrics/micrometer/src/main/java/com/datastax/oss/driver/internal/metrics/micrometer/MicrometerMetricUpdater.java b/metrics/micrometer/src/main/java/com/datastax/oss/driver/internal/metrics/micrometer/MicrometerMetricUpdater.java index 7a4a27991e3..b9507c8b7cf 100644 --- a/metrics/micrometer/src/main/java/com/datastax/oss/driver/internal/metrics/micrometer/MicrometerMetricUpdater.java +++ b/metrics/micrometer/src/main/java/com/datastax/oss/driver/internal/metrics/micrometer/MicrometerMetricUpdater.java @@ -83,7 +83,7 @@ public void updateTimer( } @Override - protected void clearMetrics() { + public void clearMetrics() { for (Meter metric : metrics.values()) { registry.remove(metric); } diff --git a/metrics/microprofile/src/main/java/com/datastax/oss/driver/internal/metrics/microprofile/MicroProfileMetricUpdater.java b/metrics/microprofile/src/main/java/com/datastax/oss/driver/internal/metrics/microprofile/MicroProfileMetricUpdater.java index a46e82ee624..df44fd69c51 100644 --- a/metrics/microprofile/src/main/java/com/datastax/oss/driver/internal/metrics/microprofile/MicroProfileMetricUpdater.java +++ b/metrics/microprofile/src/main/java/com/datastax/oss/driver/internal/metrics/microprofile/MicroProfileMetricUpdater.java @@ -83,7 +83,7 @@ public void updateTimer( } @Override - protected void clearMetrics() { + public void clearMetrics() { for (MetricT metric : metrics.keySet()) { MetricId id = getMetricId(metric); Tag[] tags = MicroProfileTags.toMicroProfileTags(id.getTags());