Copilot commented on code in PR #11141:
URL: https://github.com/apache/ozone/pull/11141#discussion_r3879527136
##########
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:
This method’s correctness relies on the invariant described in the Javadoc
(entries in the active deleted table having `seqNumMax` above the previous
snapshot create index). If that invariant is violated for any reason (corrupt
metadata, future behavior changes, or edge cases), the current logic will
return `true` (reclaimable) without any snapshot DB validation, which is the
unsafe direction. Consider adding a conservative guard such as: if `seqNumMax
<= previousSnapshotCreateIndex`, return `null` (fall back to snapshot lookup)
instead of reclaiming purely from the interval.
##########
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:
If `keyToDelete` reuses the same objectID but has `seqNumMin == null` (e.g.,
the key/version predates the feature and therefore has no interval), returning
`trxnLogIndex` can set the interval start too late. A snapshot taken before
this commit but still referencing the objectID could later fall outside
`[seqNumMin, seqNumMax)` and be incorrectly treated as not referencing the
blocks when the version is deleted, allowing unsafe reclamation. To keep
correctness during rollout, consider returning `null` (force legacy lookup
path) when `keyToDelete != null`, objectID matches, and
`keyToDelete.getSeqNumMin() == null`, or otherwise set a conservative minimum
that cannot exceed any snapshot create index that might reference the object.
##########
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:
The metric description \"required a previous snapshot read\" is stricter
than what the code currently counts (it increments whenever the interval
fast-path doesn’t short-circuit, even if later logic may not actually read a
snapshot DB in some cases). To avoid misleading operational dashboards,
consider rewording the metric to reflect what it truly measures (e.g., \"fell
back to legacy snapshot-lookup path\") or increment it only when a previous
snapshot DB read is actually performed.
--
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]
