peterxcli opened a new pull request, #11141: URL: https://github.com/apache/ozone/pull/11141
## What changes were proposed in this pull request? Deciding whether a deleted key version's blocks can be reclaimed currently requires reading the previous snapshot's RocksDB. For every entry in `deletedTable`, `ReclaimableKeyFilter.isReclaimable` performs a `getIfExist` on the active store's `snapshotRenamedTable`, a `get` on the previous snapshot's `keyTable` (including a full `KeyInfo` parse), and a block-location comparison — all to produce the answer "yes, reclaimable" in the overwhelmingly common case. This PR records an MVCC visibility interval on each key version so the decision becomes a sequence-number comparison instead of a storage lookup. Two optional fields are added to `KeyInfo`: - `seqNumMin` — the OM transaction index at which the objectID first became visible in the key table - `seqNumMax` — the transaction index at which the version moved to the deleted table (exclusive) A version is visible to a snapshot when `seqNumMin <= snapshotCreateTxnIndex < seqNumMax`. ### Only the previous snapshot has to be checked The Jira proposes an `O(S)` sorted array of every active snapshot's create sequence per bucket plus `O(D log S)` binary searches. That turns out to be unnecessary: the check is exact against the *immediately previous* snapshot alone, making it `O(1)` with no per-bucket state. `OmSnapshotManager.createOmSnapshotCheckpoint` range-deletes the bucket's `deletedTable`, `deletedDirTable` and `snapshotRenamedTable` from the active DB as soon as a checkpoint is taken. So, inductively, for any store `X` with immediately-previous path snapshot `P`, every entry in `X.deletedTable` has `seqNumMax > P.createSeq` — entries older than `P` were drained into `P`'s own copy, and entries moved in later by `SnapshotDeletingService` came from snapshots newer than `P`, which satisfy the same invariant. Given `seqNumMax > P.createSeq`, if any snapshot `S_i` satisfies `seqNumMin <= S_i.createSeq < seqNumMax`, then `S_i.createSeq < P.createSeq < seqNumMax` and `seqNumMin <= S_i.createSeq <= P.createSeq`, so `P` satisfies it too. This is also why the existing filter only consults the previous snapshot. The decision therefore reduces to, with no DB access: ```java referenced = seqNumMin <= prevSnapCreateTxnIndex && prevSnapCreateTxnIndex < seqNumMax; ``` The Jira description should be updated to drop the sorted-array design. ### Why seqNumMin is preserved rather than re-stamped The Jira's fill rule sets `seqNumMin = currentTransactionIndex` on every data-changing commit. This PR instead *preserves* `seqNumMin` when a commit reuses the existing objectID. hsync commits the same objectID repeatedly while appending blocks, and an MPU overwrite reuses the existing key's objectID. Re-stamping would let a snapshot taken between two commits fall outside the interval, and the blocks it references could then be reclaimed. Preserving the first-visible transaction of the objectID is what makes the safety argument below hold. ### Safety The fast path only short-circuits on "reclaimable". A "referenced" verdict falls through to the existing logic, which is also what computes `exclusiveSizeMap` / `exclusiveReplicatedSizeMap`, so exclusive-size accounting is byte-for-byte unchanged. The only possible behavioural delta is therefore "the interval path reclaims something the lookup path would keep". That cannot happen. If the lookup path says "not reclaimable", the key is live in `P.keyTable` with a matching objectID, so it became visible at or before `P.createSeq` — hence `seqNumMin <= P.createSeq` — and `seqNumMax`, the delete transaction, is greater than `P.createSeq`. The interval path also says "referenced". ### Scope and rollout Phase 1 covers committed key versions in `deletedTable` only. Directory reclamation, rename entries and snapshot-deletion movement are unchanged, per the Jira. MPU part deletes and the `wrapUncommittedBlocksAsPseudoKey` reclaim marker carry no `seqNumMin`, so they keep falling back; giving them empty intervals is a follow-up. Both writing the fields and using them are gated on a new `OMLayoutFeature.SNAPSHOT_RECLAIM_SEQ_NUM(12)`. Adding optional protobuf fields is wire-compatible, field presence is used rather than a `0` sentinel, and any missing metadata falls back to current behaviour — never to "safe to reclaim". No in-place migration is needed. Field 23 is deliberately skipped: it is retired (it was `expectedETag`, removed by HDDS-13919). Two counters are added to `DeletingServiceMetrics` so the optimized-versus-fallback hit rate is observable in production. ## What is the link to the Apache JIRA https://issues.apache.org/jira/browse/HDDS-15125 ## How was this patch tested? **Unit and integration tests.** 2219 existing `ozone-manager` tests and the `ozone-common` helper tests pass unchanged. New coverage: - `TestOmKeyInfo` — proto round-trip with both fields present, both absent, and partially populated, including the key-table codec path (`isOpenKey=false`) where the interval must not be dropped - `TestReclaimableKeyFilter` — interval boundaries (before, inside, exactly at `seqNumMin`, exactly at `seqNumMax`, empty interval, no previous snapshot), fallback when either field or `createTransactionInfo` is missing, and fallback before layout-feature finalization. The reclaimable cases assert via `verify(..., never())` that the previous snapshot is not read at all - `TestOMKeyCommitRequest` / `...WithFSO` — commit opens the interval, overwrite closes the previous version's interval in `deletedTable`, no interval is written before finalization, and `seqNumMin` is preserved across commits of the same objectID `checkstyle.sh` and `rat.sh` are clean. **Benchmarks.** Two `@Tag("benchmark")` harnesses are included, excluded from CI by the default test groups. Numbers below are from an Apple Silicon laptop. `ReclaimableKeyFilterBenchmark` times the two decision procedures against real RocksDB stores (median of 9 rounds, keys visited in key order to match how the service iterates): | deleted versions | referenced | lookup | interval | ratio | |---|---|---|---|---| | 10 000 | 0 % | 2.8 µs | 1.9 ns | 1445x | | 10 000 | 50 % | 7.5 µs | 2.0 ns | 3757x | | 10 000 | 100 % | 6.3 µs | 7.6 ns | 823x | | 50 000 | 0 % | 1.9 µs | 9.3 ns | 207x | | 50 000 | 50 % | 5.5 µs | 11.5 ns | 477x | | 50 000 | 100 % | 6.3 µs | 7.8 ns | 805x | `KeyDeletingServiceBenchmark` times a whole `getPendingDeletionKeys` pass on `MiniOzoneCluster`. Both arms run the same binary and differ only in the OM layout version they were initialized at, so one falls back to previous-snapshot lookups and the other uses the interval. Half the deleted versions are referenced by the snapshot. Three clusters per arm, interleaved: ``` previous snapshot lookup : min 443.0 median 468.3 max 481.9 ms (221.5 us/key at min) visibility interval : min 328.2 median 428.6 max 514.7 ms (164.1 us/key at min) ``` Median ratio **1.09x**, roughly 20 µs saved per key, about 1 s per 50 000-key service run at the default `ozone.key.deleting.limit.per.task`. The arms' spreads overlap at n=3, so this is indicative rather than precise. The same test asserts the purged-key count in both arms: both returned exactly 1000 of 2000, confirming the two paths reach identical verdicts on a real cluster. The gap between the two benchmarks is itself worth flagging for a follow-up. `ReclaimableFilter.apply()` calls `validateExistingLastNSnapshotsInChain` **twice per key**, and each call walks two snapshots doing `SnapshotUtils.getSnapshotInfo` plus `TransactionInfo.readTransactionInfo` — the latter uses `getSkipCache`, which bypasses the table cache and reads RocksDB directly. That is roughly four uncached reads per deleted key version for chain revalidation, against the two reads this PR removes, and it dominates the pass. The chain cannot change between keys within a single iteration, so hoisting that revalidation is likely worth more than this change. Generated-by: Claude Code (Opus 5) -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
