From 37ec4dab540c3dc515cb65464036a2da8aa851f6 Mon Sep 17 00:00:00 2001 From: Anurag Parvatikar Date: Thu, 1 Oct 2026 13:09:30 +0530 Subject: [PATCH 1/3] HDDS-16654. Replace usage of deprecated finalize() in OM --- .../apache/hadoop/ozone/om/OmSnapshot.java | 32 ++++--- .../ozone/om/TestOmSnapshotLeakDetection.java | 91 +++++++++++++++++++ 2 files changed, 112 insertions(+), 11 deletions(-) create mode 100644 hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/TestOmSnapshotLeakDetection.java diff --git a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmSnapshot.java b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmSnapshot.java index 6d3a56f40ed0..5d9bf51ba455 100644 --- a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmSnapshot.java +++ b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmSnapshot.java @@ -29,6 +29,8 @@ import java.util.stream.Collectors; import org.apache.hadoop.hdds.client.RatisReplicationConfig; import org.apache.hadoop.hdds.protocol.proto.HddsProtos; +import org.apache.hadoop.hdds.utils.LeakDetector; +import org.apache.hadoop.hdds.utils.db.DBStore; import org.apache.hadoop.ozone.OzoneAcl; import org.apache.hadoop.ozone.audit.AuditLogger; import org.apache.hadoop.ozone.audit.AuditLoggerType; @@ -48,6 +50,7 @@ import org.apache.hadoop.ozone.security.acl.OzoneObj; import org.apache.hadoop.ozone.security.acl.OzoneObjInfo; import org.apache.hadoop.util.Time; +import org.apache.ratis.util.UncheckedAutoCloseable; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -72,6 +75,8 @@ public class OmSnapshot implements IOmMetadataReader, Closeable { private static final AuditLogger AUDIT = new AuditLogger( AuditLoggerType.OMLOGGER); + private static final LeakDetector LEAK_DETECTOR = new LeakDetector(OmSnapshot.class.getName()); + private final OmMetadataReader omMetadataReader; private final String volumeName; private final String bucketName; @@ -80,6 +85,8 @@ public class OmSnapshot implements IOmMetadataReader, Closeable { // To access snapshot checkpoint DB metadata private final OMMetadataManager omMetadataManager; private final KeyManager keyManager; + /** Warns if this snapshot is garbage collected without being closed. Assigned in the constructor. */ + private final UncheckedAutoCloseable leakTracker; public OmSnapshot(KeyManager keyManager, PrefixManager prefixManager, @@ -100,6 +107,7 @@ public OmSnapshot(KeyManager keyManager, this.snapshotID = snapshotID; this.keyManager = keyManager; this.omMetadataManager = keyManager.getMetadataManager(); + this.leakTracker = LEAK_DETECTOR.track(this, newLeakReporter(omMetadataManager.getStore(), snapshotName)); } @Override @@ -345,19 +353,21 @@ public UUID getSnapshotID() { public void close() throws IOException { // Close DB omMetadataManager.getStore().close(); + // Closed properly: stop tracking so the leak reporter does not fire at GC. + leakTracker.close(); } - @Override - protected void finalize() throws Throwable { - // Verify that the DB handle has been closed, log warning otherwise - // https://softwareengineering.stackexchange.com/a/288724 - if (!omMetadataManager.getStore().isClosed()) { - LOG.warn("{} is not closed properly. snapshotName: {}", - // Print hash code for debugging - omMetadataManager.getStore().toString(), - snapshotName); - } - super.finalize(); + /** + * @return a leak reporter that captures only the {@code store} and {@code snapshotName} objects, + * never the {@link OmSnapshot} itself. + */ + static Runnable newLeakReporter(DBStore store, String snapshotName) { + return () -> { + if (!store.isClosed()) { + // Print hash code for debugging + LOG.warn("{} is not closed properly. snapshotName: {}", store, snapshotName); + } + }; } @VisibleForTesting diff --git a/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/TestOmSnapshotLeakDetection.java b/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/TestOmSnapshotLeakDetection.java new file mode 100644 index 000000000000..f0be8a05483b --- /dev/null +++ b/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/TestOmSnapshotLeakDetection.java @@ -0,0 +1,91 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.hadoop.ozone.om; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; + +import java.util.UUID; +import org.apache.hadoop.hdds.utils.db.DBStore; +import org.apache.hadoop.ozone.security.acl.IAccessAuthorizer; +import org.apache.ozone.test.GenericTestUtils.LogCapturer; +import org.junit.jupiter.api.Test; + +/** + * Test {@link OmSnapshot}'s leak detection: the snapshot registers with a shared LeakDetector + * and warns if it is garbage collected while its underlying store is still open. + */ +class TestOmSnapshotLeakDetection { + + /** The reporter warns only when the store was not closed. Exercised directly with a mocked store. */ + @Test + void reporterWarnsWhenStoreNotClosed() { + DBStore store = mock(DBStore.class); + when(store.isClosed()).thenReturn(false); + try (LogCapturer logs = LogCapturer.captureLogs(OmSnapshot.class)) { + OmSnapshot.newLeakReporter(store, "snap-1").run(); + assertThat(logs.getOutput()).contains("is not closed properly. snapshotName: snap-1"); + } + } + + @Test + void reporterSilentWhenStoreClosed() { + DBStore store = mock(DBStore.class); + when(store.isClosed()).thenReturn(true); + try (LogCapturer logs = LogCapturer.captureLogs(OmSnapshot.class)) { + OmSnapshot.newLeakReporter(store, "snap-1").run(); + assertThat(logs.getOutput()).doesNotContain("is not closed properly"); + } + } + + /** + * Drive an actual {@link OmSnapshot} instance through GC without closing it and verify the leak + * is detected. Collaborators (including the {@link DBStore}) are mocked, so no metadata store is + * opened; this exercises the constructor's LeakDetector registration and the GC-triggered report. + */ + @Test + void leakDetectedForUnclosedSnapshot() throws Exception { + DBStore store = mock(DBStore.class); + when(store.isClosed()).thenReturn(false); + OMMetadataManager metadataManager = mock(OMMetadataManager.class); + when(metadataManager.getStore()).thenReturn(store); + KeyManager keyManager = mock(KeyManager.class); + when(keyManager.getMetadataManager()).thenReturn(metadataManager); + OzoneManager ozoneManager = mock(OzoneManager.class); + IAccessAuthorizer authorizer = mock(IAccessAuthorizer.class); + when(ozoneManager.getAccessAuthorizer()).thenReturn(authorizer); + when(authorizer.isNative()).thenReturn(false); + PrefixManager prefixManager = mock(PrefixManager.class); + + try (LogCapturer logs = LogCapturer.captureLogs(OmSnapshot.class)) { + OmSnapshot snapshot = new OmSnapshot(keyManager, prefixManager, ozoneManager, + "vol", "bucket", "snap-1", UUID.randomUUID()); + assertThat(snapshot).isNotNull(); + + // Drop the only strong reference; the reporter captures the store, not the snapshot, + // so it becomes collectible. The report runs asynchronously on the LeakDetector thread. + snapshot = null; + for (int i = 0; i < 50 && !logs.getOutput().contains("is not closed properly"); i++) { + System.gc(); + Thread.sleep(100); + } + assertThat(logs.getOutput()).contains("is not closed properly. snapshotName: snap-1"); + } + } +} From bcfc1d68b45ca7e3abcaf7553651af3c472bb522 Mon Sep 17 00:00:00 2001 From: Anurag Parvatikar Date: Fri, 2 Oct 2026 22:36:51 +0530 Subject: [PATCH 2/3] Address review comments --- .../apache/hadoop/ozone/om/OmSnapshot.java | 26 ++++----- .../ozone/om/TestOmSnapshotLeakDetection.java | 58 +++++++++---------- 2 files changed, 39 insertions(+), 45 deletions(-) diff --git a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmSnapshot.java b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmSnapshot.java index 5d9bf51ba455..6adad18c2766 100644 --- a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmSnapshot.java +++ b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmSnapshot.java @@ -30,7 +30,6 @@ import org.apache.hadoop.hdds.client.RatisReplicationConfig; import org.apache.hadoop.hdds.protocol.proto.HddsProtos; import org.apache.hadoop.hdds.utils.LeakDetector; -import org.apache.hadoop.hdds.utils.db.DBStore; import org.apache.hadoop.ozone.OzoneAcl; import org.apache.hadoop.ozone.audit.AuditLogger; import org.apache.hadoop.ozone.audit.AuditLoggerType; @@ -107,7 +106,8 @@ public OmSnapshot(KeyManager keyManager, this.snapshotID = snapshotID; this.keyManager = keyManager; this.omMetadataManager = keyManager.getMetadataManager(); - this.leakTracker = LEAK_DETECTOR.track(this, newLeakReporter(omMetadataManager.getStore(), snapshotName)); + this.leakTracker = LEAK_DETECTOR.track(this, + newLeakReporter(omMetadataManager.getStore().toString(), snapshotName)); } @Override @@ -351,23 +351,21 @@ public UUID getSnapshotID() { @Override public void close() throws IOException { - // Close DB - omMetadataManager.getStore().close(); - // Closed properly: stop tracking so the leak reporter does not fire at GC. - leakTracker.close(); + try { + // Close DB + omMetadataManager.getStore().close(); + } finally { + // Closed properly: stop tracking so the leak reporter does not fire at GC. + leakTracker.close(); + } } /** - * @return a leak reporter that captures only the {@code store} and {@code snapshotName} objects, + * @return a leak reporter that captures only the store's hashcode and the snapshot name, * never the {@link OmSnapshot} itself. */ - static Runnable newLeakReporter(DBStore store, String snapshotName) { - return () -> { - if (!store.isClosed()) { - // Print hash code for debugging - LOG.warn("{} is not closed properly. snapshotName: {}", store, snapshotName); - } - }; + static Runnable newLeakReporter(String storeHashCode, String snapshotName) { + return () -> LOG.warn("{} is not closed properly. snapshotName: {}", storeHashCode, snapshotName); } @VisibleForTesting diff --git a/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/TestOmSnapshotLeakDetection.java b/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/TestOmSnapshotLeakDetection.java index f0be8a05483b..432ddf69adc3 100644 --- a/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/TestOmSnapshotLeakDetection.java +++ b/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/TestOmSnapshotLeakDetection.java @@ -33,36 +33,44 @@ */ class TestOmSnapshotLeakDetection { - /** The reporter warns only when the store was not closed. Exercised directly with a mocked store. */ + /** + * Drive an actual {@link OmSnapshot} instance through GC without closing it and verify the leak + * is detected. Collaborators (including the {@link DBStore}) are mocked, so no metadata store is + * opened; this exercises the constructor's LeakDetector registration and the GC-triggered report. + */ @Test - void reporterWarnsWhenStoreNotClosed() { - DBStore store = mock(DBStore.class); - when(store.isClosed()).thenReturn(false); + void leakDetectedForUnclosedSnapshot() throws Exception { try (LogCapturer logs = LogCapturer.captureLogs(OmSnapshot.class)) { - OmSnapshot.newLeakReporter(store, "snap-1").run(); + OmSnapshot snapshot = newSnapshotWithMockedStore(); + assertThat(snapshot).isNotNull(); + + // Drop the only strong reference; the reporter captures no reference back to the snapshot, + // so it becomes collectible. The report runs asynchronously on the LeakDetector thread. + snapshot = null; + for (int i = 0; i < 50 && !logs.getOutput().contains("is not closed properly"); i++) { + System.gc(); + Thread.sleep(100); + } assertThat(logs.getOutput()).contains("is not closed properly. snapshotName: snap-1"); } } + /** Closing the snapshot stops the leak tracker, so a GC afterwards must not report a leak. */ @Test - void reporterSilentWhenStoreClosed() { - DBStore store = mock(DBStore.class); - when(store.isClosed()).thenReturn(true); + void closedSnapshotDoesNotReportLeak() throws Exception { try (LogCapturer logs = LogCapturer.captureLogs(OmSnapshot.class)) { - OmSnapshot.newLeakReporter(store, "snap-1").run(); + OmSnapshot snapshot = newSnapshotWithMockedStore(); + snapshot.close(); + + snapshot = null; + System.gc(); + Thread.sleep(100); assertThat(logs.getOutput()).doesNotContain("is not closed properly"); } } - /** - * Drive an actual {@link OmSnapshot} instance through GC without closing it and verify the leak - * is detected. Collaborators (including the {@link DBStore}) are mocked, so no metadata store is - * opened; this exercises the constructor's LeakDetector registration and the GC-triggered report. - */ - @Test - void leakDetectedForUnclosedSnapshot() throws Exception { + private static OmSnapshot newSnapshotWithMockedStore() { DBStore store = mock(DBStore.class); - when(store.isClosed()).thenReturn(false); OMMetadataManager metadataManager = mock(OMMetadataManager.class); when(metadataManager.getStore()).thenReturn(store); KeyManager keyManager = mock(KeyManager.class); @@ -73,19 +81,7 @@ void leakDetectedForUnclosedSnapshot() throws Exception { when(authorizer.isNative()).thenReturn(false); PrefixManager prefixManager = mock(PrefixManager.class); - try (LogCapturer logs = LogCapturer.captureLogs(OmSnapshot.class)) { - OmSnapshot snapshot = new OmSnapshot(keyManager, prefixManager, ozoneManager, - "vol", "bucket", "snap-1", UUID.randomUUID()); - assertThat(snapshot).isNotNull(); - - // Drop the only strong reference; the reporter captures the store, not the snapshot, - // so it becomes collectible. The report runs asynchronously on the LeakDetector thread. - snapshot = null; - for (int i = 0; i < 50 && !logs.getOutput().contains("is not closed properly"); i++) { - System.gc(); - Thread.sleep(100); - } - assertThat(logs.getOutput()).contains("is not closed properly. snapshotName: snap-1"); - } + return new OmSnapshot(keyManager, prefixManager, ozoneManager, + "vol", "bucket", "snap-1", UUID.randomUUID()); } } From 5f134d66944222fbd6e7d4f21037efd255a7bf11 Mon Sep 17 00:00:00 2001 From: "Doroszlai, Attila" <6454655+adoroszlai@users.noreply.github.com> Date: Sun, 4 Oct 2026 09:21:58 +0200 Subject: [PATCH 3/3] Apply suggestions from code review Co-authored-by: Chi-Hsuan Huang --- .../main/java/org/apache/hadoop/ozone/om/OmSnapshot.java | 6 +++--- .../apache/hadoop/ozone/om/TestOmSnapshotLeakDetection.java | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmSnapshot.java b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmSnapshot.java index 6adad18c2766..3e59092c74ff 100644 --- a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmSnapshot.java +++ b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmSnapshot.java @@ -361,11 +361,11 @@ public void close() throws IOException { } /** - * @return a leak reporter that captures only the store's hashcode and the snapshot name, + * @return a leak reporter that captures only the store description and the snapshot name, * never the {@link OmSnapshot} itself. */ - static Runnable newLeakReporter(String storeHashCode, String snapshotName) { - return () -> LOG.warn("{} is not closed properly. snapshotName: {}", storeHashCode, snapshotName); + private static Runnable newLeakReporter(String storeDescription, String snapshotName) { + return () -> LOG.warn("{} is not closed properly. snapshotName: {}", storeDescription, snapshotName); } @VisibleForTesting diff --git a/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/TestOmSnapshotLeakDetection.java b/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/TestOmSnapshotLeakDetection.java index 432ddf69adc3..5162b33ccc60 100644 --- a/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/TestOmSnapshotLeakDetection.java +++ b/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/TestOmSnapshotLeakDetection.java @@ -29,7 +29,7 @@ /** * Test {@link OmSnapshot}'s leak detection: the snapshot registers with a shared LeakDetector - * and warns if it is garbage collected while its underlying store is still open. + * and warns if it is garbage collected without being closed. */ class TestOmSnapshotLeakDetection {