HDDS-15888. Stamp lastTransactionInfo on snapshot deletion to avoid premature GC#10788
Draft
smengcl wants to merge 2 commits into
Draft
HDDS-15888. Stamp lastTransactionInfo on snapshot deletion to avoid premature GC#10788smengcl wants to merge 2 commits into
smengcl wants to merge 2 commits into
Conversation
…t GC waits for the deletion to be flushed
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes a snapshot-GC durability gap in Ozone Manager by stamping lastTransactionInfo during snapshot deletion, ensuring flush-gating logic (areSnapshotChangesFlushedToDB) correctly defers snapshot GC until the delete is durably flushed.
Changes:
- Stamp
SnapshotInfo.lastTransactionInfoinOMSnapshotDeleteRequest.validateAndUpdateCacheon deletion. - Add a regression test ensuring snapshot deletion is not reported “flushed” until the transaction marker advances.
- Add boundary tests documenting/covering reclaimability behavior when the in-memory snapshot chain is empty.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/snapshot/OMSnapshotDeleteRequest.java | Stamp deletion lastTransactionInfo so flush gates correctly defer snapshot GC. |
| hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/request/snapshot/TestOMSnapshotDeleteRequest.java | Adds regression coverage for the flush-marker check around snapshot deletion. |
| hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/snapshot/filter/TestReclaimableKeyFilter.java | Adds boundary tests around reclaimability when the snapshot chain appears empty. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+284
to
+292
| /** | ||
| * Repro for the flush-lag reclamation window (companion to | ||
| * TestReclaimableKeyFilter#testKeyNotReclaimableWhenChainEmptyingPurgeIsUnflushed): snapshot deletion | ||
| * updates only status and deletionTime and never stamps lastTransactionInfo (unlike create, moveTableKeys, | ||
| * purge and key/dir purge), so areSnapshotChangesFlushedToDB() keys off the stale create-time stamp and | ||
| * reports an applied-but-unflushed deletion as flushed. SnapshotDeletingService#shouldIgnoreSnapshot relies | ||
| * on that method to defer processing until a snapshot's latest change is durable; the missing stamp lets | ||
| * the deletion be processed (moveTableKeys + purge submitted) before the double buffer has flushed it. | ||
| */ |
Comment on lines
+325
to
+327
| assertFalse(OmSnapshotManager.areSnapshotChangesFlushedToDB(getOmMetadataManager(), key), | ||
| "snapshot deletion applied at index 2 is unflushed (marker at the create transaction) but is" | ||
| + " reported flushed because OMSnapshotDeleteRequest does not stamp lastTransactionInfo"); |
Comment on lines
+344
to
+345
| // ...but the purge is not flushed: the applied/cache view of snapshotInfoTable has no row while the | ||
| // on-disk (skip-cache) view still holds the row -- as DELETED, guaranteed by the SDS flush gate. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What changes were proposed in this pull request?
Unlike create/moveTableKeys/purge,
OMSnapshotDeleteRequestnever stampslastTransactionInfo.areSnapshotChangesFlushedToDB()therefore reads the stale create-time stamp and reports an unflushed deletion as flushed, soSnapshotDeletingServicedoes not defer it and can purge the snapshot while the delete and purge exist only in the double buffer. The purge empties the in-memory chain,ReclaimableFilterfinds no previous snapshot (areSnapshotChangesFlushedToDB(null)is vacuously true), andKeyDeletingServicehands blocks to SCM irreversibly while the on-disksnapshotInfoTablestill shows the snapshotACTIVEand referencing them.Severity
Low. The deletion was user-requested and Ratis-acked, DELETED snapshots cannot be unmarked, and crash recovery replays idx 3-5, so no supported flow reads the lost blocks. The inconsistency surfaces only via recovery that drops acked transactions (RocksDB restored without the matching Ratis log). Still worth fixing because both existing flush gates exist precisely to keep irreversible block deletion behind locally durable state, and the missing stamp silently defeats them. Found by model-checking the snapshot chain with TLA+, then reproduced in unit tests.
Fix
Stamp
lastTransactionInfoinOMSnapshotDeleteRequest.validateAndUpdateCache, matching the other snapshot requests. The existing SDS gate then defers move/purge until the deletion is durable, so a chain-removed snapshot is at worst DELETED on disk, never ACTIVE.Cost: snapshot GC waits at most one double-buffer flush cycle.
What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-15888
How was this patch tested?
TestOMSnapshotDeleteRequest#testSnapshotDeleteIsNotReportedFlushedUntilFlushed: fails without the fix, passes with it.TestReclaimableKeyFilterboundary tests: empty-chain reclamation is only legal with the on-disk row absent or DELETED; javadoc records the invariant.Generated-by: Claude Code (Fable 5)