HDDS-16654. Replace usage of deprecated finalize() in OM - #11376
anuragp010 wants to merge 2 commits into
Conversation
chihsuan
left a comment
chihsuan
left a comment
There was a problem hiding this comment.
Thanks for working on this! @anuragp010 Looks good overall, just two small notes. 👍
| * opened; this exercises the constructor's LeakDetector registration and the GC-triggered report. | ||
| */ | ||
| @Test | ||
| void leakDetectedForUnclosedSnapshot() throws Exception { |
There was a problem hiding this comment.
Should we also add a test for the closed case? Right now the tests still pass even without leakTracker.close().
There was a problem hiding this comment.
Thanks @chihsuan ! Yes that's a good point. I have added it.
| // Close DB | ||
| omMetadataManager.getStore().close(); | ||
| // Closed properly: stop tracking so the leak reporter does not fire at GC. | ||
| leakTracker.close(); |
There was a problem hiding this comment.
nit: Could we use try/finally here, like OzoneClient#close? That way the tracker is still released if the store close throws.
There was a problem hiding this comment.
Makes sense - have added it.
adoroszlai
left a comment
adoroszlai
left a comment
There was a problem hiding this comment.
Thanks @anuragp010 for the patch.
| return () -> { | ||
| if (!store.isClosed()) { | ||
| // Print hash code for debugging | ||
| LOG.warn("{} is not closed properly. snapshotName: {}", store, snapshotName); | ||
| } | ||
| }; |
There was a problem hiding this comment.
I don't think we should keep a strong reference to store in the leak reporter.
- I think checking
store.isClosed()is unnecessary.LeakTrackerdiscards 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.
There was a problem hiding this comment.
Thanks @adoroszlai ! Right, makes sense. I have addressed this.
3 participants
What changes were proposed in this pull request?
OmSnapshotoverrodeObject.finalize()to warn if itsDBStorewasn't closed before GC. Butfinalize()is deprecated for removal (JEP 421). This PR replaces it withLeakDetector, aReferenceQueue-based leak tracker, used in the same way byManagedRocksObjectUtils. A staticLEAK_DETECTORtracks eachOmSnapshotinstance viaLEAK_DETECTOR.track(this, reporter). If the instance is dropped unclosed, the detector's background thread reports the same warning the oldfinalize()did, once the object is actually collected.Generated with Claude Code.
What is the link to the Apache JIRA
HDDS-16654
How was this patch tested?