peterxcli commented on code in PR #11141:
URL: https://github.com/apache/ozone/pull/11141#discussion_r3879715772
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyRequest.java:
##########
@@ -1174,6 +1175,27 @@ protected String getDBMultipartOpenKey(String
volumeName, String bucketName,
.getMultipartKey(volumeName, bucketName, keyName, uploadID);
}
+ /**
+ * Resolves the start of the MVCC visibility interval for the key version
being committed.
+ *
+ * <p>hsync commits the same objectID repeatedly while appending blocks, and
an MPU overwrite
+ * reuses the existing key's objectID. The interval start must stay at the
transaction where the
+ * objectID first became visible in the key table, otherwise a snapshot
taken between two commits
+ * would fall outside the interval and its blocks could be reclaimed.
Returns null before the
+ * layout feature is finalized, which leaves reclaimability on the
previous-snapshot lookup path.
+ */
+ protected static Long resolveSeqNumMin(OzoneManager ozoneManager, OmKeyInfo
keyToDelete,
+ OmKeyInfo committedKeyInfo, long trxnLogIndex) {
+ if
(!ozoneManager.getVersionManager().isAllowed(OMLayoutFeature.SNAPSHOT_RECLAIM_SEQ_NUM))
{
+ return null;
+ }
Review Comment:
Good catch — this is a real correctness bug, not just a rollout wrinkle.
Concretely:
1. Key committed at txn 100 with objectID 5 before finalization, so no
interval.
2. Snapshot S created at txn 150; `S.keyTable` holds that version.
3. Layout feature finalized.
4. Same objectID re-committed at txn 200 (hsync re-commit, or an MPU
overwrite, both of which
carry the key table objectID forward). Old code stamped `seqNumMin = 200`.
5. Version deleted at txn 300, giving interval `[200, 300)`.
6. Checked against S at 150: not inside the interval, so reclaimable — while
S still references
the blocks. The lookup path would have matched on objectID and kept them.
Fixed by returning `keyToDelete.getSeqNumMin()` whenever the objectID is
carried over, which
collapses both cases: preserve when it exists, and return null when it does
not, leaving the
version on the lookup path. `prepareKeyForDelete` then leaves `seqNumMax`
unset too, so nothing
downstream sees a partial interval. Self-healing: any version written under
a fresh objectID gets
a real interval.
Covered in `testSeqNumMinPreservedAcrossCommitsOfSameObjectId`.
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/snapshot/filter/ReclaimableKeyFilter.java:
##########
@@ -116,6 +134,50 @@ protected Boolean isReclaimable(Table.KeyValue<String,
OmKeyInfo> deletedKeyInfo
return false;
}
+ /**
+ * Decides reclaimability from the key version's MVCC visibility interval,
without reading any
+ * snapshot DB.
+ *
+ * <p>A snapshot sees this key version when
+ * {@code seqNumMin <= snapshotCreateTxnIndex < seqNumMax}. Only the
immediately previous snapshot
+ * in the chain has to be checked: {@link
OmSnapshotManager#createOmSnapshotCheckpoint} drains the
+ * bucket's deletedTable from the active DB as soon as a snapshot checkpoint
is taken, so every
+ * entry reachable here has {@code seqNumMax} greater than the previous
snapshot's create index.
+ * Any older snapshot inside the interval therefore implies the previous
snapshot is inside it
+ * too.
+ *
+ * @return {@code TRUE} when no snapshot can see this version, {@code FALSE}
when the previous
+ * snapshot can see it, and {@code null} when the interval metadata
is absent and the
+ * caller must fall back to previous snapshot lookups.
+ */
+ private Boolean isReclaimableByVisibilityInterval(OmKeyInfo deletedKeyInfo)
throws IOException {
+ Long seqNumMin = deletedKeyInfo.getSeqNumMin();
+ Long seqNumMax = deletedKeyInfo.getSeqNumMax();
+ if (!intervalOptimizationEnabled || seqNumMin == null || seqNumMax ==
null) {
+ return null;
+ }
+ SnapshotInfo previousSnapshotInfo = getPreviousSnapshotInfo(1);
+ if (previousSnapshotInfo == null) {
+ // No previous snapshot in the chain, so no snapshot can reference this
key version.
+ return true;
Review Comment:
Agreed, and worth doing — the whole "check only the previous snapshot"
argument rests on that
invariant, so it should be asserted rather than assumed.
`seqNumMax <= previousSnapshotCreateIndex` is exactly the signature of the
dangerous case: it means
the entry closed before the previous snapshot was created, which under the
drain-at-checkpoint
behaviour cannot happen, and which is precisely when an *older* snapshot
could sit inside the
interval unchecked. Old code answered "reclaimable" there without validation.
Added the guard, and with `seqNumMax > previousSnapshotCreateIndex` now
established the verdict
reduces to `seqNumMin > previousSnapshotCreateIndex`. A tripped guard is
observable: it returns
null, so the entry counts under the fallback metric.
Two tests moved as a result — the old `seqNumMax`-exclusive and
empty-interval cases both construct
intervals that close at or before the previous snapshot, so they are now
folded into
`testIntervalClosingBeforePreviousSnapshotFallsBackToSnapshotLookup`, which
asserts fallback.
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/DeletingServiceMetrics.java:
##########
@@ -70,6 +70,15 @@ public final class DeletingServiceMetrics {
private MutableGaugeLong numKeysPurged;
@Metric("Total no. of rename entries purged")
private MutableGaugeLong numRenameEntriesPurged;
+ /*
+ * Reclaimability decision source metrics. Tracks how often a deleted key
version could be judged
+ * from its MVCC visibility interval versus how often the previous snapshot
had to be read.
+ */
+ @Metric("Total no. of deleted key versions judged reclaimable from their
visibility interval, "
+ + "with no previous snapshot read")
Review Comment:
Correct — the counter increments when the interval fast path declines, and
the lookup path can
still return without touching a snapshot DB (for example
`getPreviousSnapshotKeyInfo` returns early
when there is no previous key manager).
Reworded to describe the path rather than assert a read:
"Total no. of deleted key versions that fell back to the previous snapshot
lookup path".
--
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]
