You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@ozone.apache.org by "anuragp010 (via GitHub)" <gi...@apache.org> on 2026/10/01 11:04:07 UTC
[PR] HDDS-16654. Replace usage of deprecated finalize() in OM [ozone]
anuragp010 opened a new pull request, #11376:
URL: https://github.com/apache/ozone/pull/11376
## What changes were proposed in this pull request?
`OmSnapshot` overrode `Object.finalize()` to warn if its `DBStore` wasn't closed before GC. But `finalize()` is deprecated for removal ([JEP 421](https://openjdk.org/jeps/421)). This PR replaces it with `LeakDetector`, a `ReferenceQueue`-based leak tracker, used in the same way by `ManagedRocksObjectUtils`. A static `LEAK_DETECTOR` tracks each `OmSnapshot` instance via `LEAK_DETECTOR.track(this, reporter)`. If the instance is dropped unclosed, the detector's background thread reports the same warning the old `finalize()` did, once the object is actually collected.
## What is the link to the Apache JIRA
[HDDS-16654](https://issues.apache.org/jira/browse/HDDS-16654)
## How was this patch tested?
1. Newly added Unit Tests
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-16654. Replace usage of deprecated finalize() in OM [ozone]
Posted by "anuragp010 (via GitHub)" <gi...@apache.org>.
anuragp010 commented on code in PR #11376:
URL: https://github.com/apache/ozone/pull/11376#discussion_r4168009383
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmSnapshot.java:
##########
@@ -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);
+ }
+ };
Review Comment:
Thanks @adoroszlai ! Right, makes sense. I have addressed this.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-16654. Replace usage of deprecated finalize() in OM [ozone]
Posted by "adoroszlai (via GitHub)" <gi...@apache.org>.
adoroszlai commented on PR #11376:
URL: https://github.com/apache/ozone/pull/11376#issuecomment-5978524929
Thanks @anuragp010 for the patch, @chihsuan for the review.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-16654. Replace usage of deprecated finalize() in OM [ozone]
Posted by "adoroszlai (via GitHub)" <gi...@apache.org>.
adoroszlai commented on code in PR #11376:
URL: https://github.com/apache/ozone/pull/11376#discussion_r4156698515
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmSnapshot.java:
##########
@@ -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);
+ }
+ };
Review Comment:
I don't think we should keep a strong reference to `store` in the leak reporter.
- I think checking `store.isClosed()` is unnecessary. `LeakTracker` discards properly closed instances and will not call the reporter.
- For "print hash code for debugging", we should get that eagerly and keep reference only to the string to be included in the message.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-16654. Replace usage of deprecated finalize() in OM [ozone]
Posted by "chihsuan (via GitHub)" <gi...@apache.org>.
chihsuan commented on code in PR #11376:
URL: https://github.com/apache/ozone/pull/11376#discussion_r4155004547
##########
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 {
Review Comment:
Should we also add a test for the closed case? Right now the tests still pass even without `leakTracker.close()`.
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmSnapshot.java:
##########
@@ -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();
Review Comment:
nit: Could we use `try`/`finally` here, like [`OzoneClient#close`](https://github.com/apache/ozone/blob/4a832912feae49664e05479d0ea9ba2c182e905e/hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/OzoneClient.java#L121-L127)? That way the tracker is still released if the store close throws.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-16654. Replace usage of deprecated finalize() in OM [ozone]
Posted by "anuragp010 (via GitHub)" <gi...@apache.org>.
anuragp010 commented on code in PR #11376:
URL: https://github.com/apache/ozone/pull/11376#discussion_r4168000574
##########
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 {
Review Comment:
Thanks @chihsuan ! Yes that's a good point. I have added it.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-16654. Replace usage of deprecated finalize() in OM [ozone]
Posted by "anuragp010 (via GitHub)" <gi...@apache.org>.
anuragp010 commented on PR #11376:
URL: https://github.com/apache/ozone/pull/11376#issuecomment-5981450079
Thanks @chihsuan and @adoroszlai !
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-16654. Replace usage of deprecated finalize() in OM [ozone]
Posted by "anuragp010 (via GitHub)" <gi...@apache.org>.
anuragp010 commented on code in PR #11376:
URL: https://github.com/apache/ozone/pull/11376#discussion_r4168002488
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmSnapshot.java:
##########
@@ -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();
Review Comment:
Makes sense - have added it.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-16654. Replace usage of deprecated finalize() in OM [ozone]
Posted by "adoroszlai (via GitHub)" <gi...@apache.org>.
adoroszlai merged PR #11376:
URL: https://github.com/apache/ozone/pull/11376
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-16654. Replace usage of deprecated finalize() in OM [ozone]
Posted by "chihsuan (via GitHub)" <gi...@apache.org>.
chihsuan commented on code in PR #11376:
URL: https://github.com/apache/ozone/pull/11376#discussion_r4173915612
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmSnapshot.java:
##########
@@ -343,21 +351,21 @@ public UUID getSnapshotID() {
@Override
public void close() throws IOException {
- // Close DB
- omMetadataManager.getStore().close();
+ try {
+ // Close DB
+ omMetadataManager.getStore().close();
+ } finally {
+ // 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 store's hashcode 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);
Review Comment:
nit: Since the tests no longer call it, I think it can be private now. The parameter name could also be clearer, see the suggestion.
```suggestion
/**
* @return a leak reporter that captures only the store description and the snapshot name,
* never the {@link OmSnapshot} itself.
*/
private static Runnable newLeakReporter(String storeDescription, String snapshotName) {
return () -> LOG.warn("{} is not closed properly. snapshotName: {}", storeDescription, snapshotName);
```
##########
hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/TestOmSnapshotLeakDetection.java:
##########
@@ -0,0 +1,87 @@
+/*
+ * 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.
Review Comment:
nit: This description is outdated. The store state is no longer checked, so it now warns whenever `close()` was not called.
```suggestion
* Test {@link OmSnapshot}'s leak detection: the snapshot registers with a shared LeakDetector
* and warns if it is garbage collected without being closed.
```
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org