Copilot commented on code in PR #10788:
URL: https://github.com/apache/ozone/pull/10788#discussion_r3600779273
##########
hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/request/snapshot/TestOMSnapshotDeleteRequest.java:
##########
@@ -277,6 +281,52 @@ public void testEntryExist() throws Exception {
assertEquals(1, getOmMetrics().getNumSnapshotDeleteFails());
}
+ /**
+ * 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.
+ */
Review Comment:
The Javadoc references
`TestReclaimableKeyFilter#testKeyNotReclaimableWhenChainEmptyingPurgeIsUnflushed`,
but that test method does not exist in the current tree. It also describes the
missing `lastTransactionInfo` stamp as current behavior, but this PR changes
that behavior. Please update the comment to reference an existing companion
test (or just the class) and phrase this as a regression for the pre-fix
behavior.
##########
hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/request/snapshot/TestOMSnapshotDeleteRequest.java:
##########
@@ -277,6 +281,52 @@ public void testEntryExist() throws Exception {
assertEquals(1, getOmMetrics().getNumSnapshotDeleteFails());
}
+ /**
+ * 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.
+ */
+ @Test
+ public void testSnapshotDeleteIsNotReportedFlushedUntilFlushed() throws
Exception {
+ when(getOzoneManager().isAdmin(any())).thenReturn(true);
+ String key = SnapshotInfo.getTableKey(getVolumeName(), getBucketName(),
snapshotName);
+
+ // Create the snapshot at transaction index 1; validateAndUpdateCache
stamps lastTransactionInfo.
+ OMRequest createRequest = createSnapshotRequest(getVolumeName(),
getBucketName(), snapshotName);
+ OMSnapshotCreateRequest omSnapshotCreateRequest =
+ TestOMSnapshotCreateRequest.doPreExecute(createRequest,
getOzoneManager());
+ omSnapshotCreateRequest.validateAndUpdateCache(getOzoneManager(), 1L);
+ SnapshotInfo snapshotInfo =
getOmMetadataManager().getSnapshotInfoTable().get(key);
+ assertNotNull(snapshotInfo);
+ assertNotNull(snapshotInfo.getLastTransactionInfo(), "sanity: create
stamps lastTransactionInfo");
+
+ // The double buffer flushes through the create transaction: persist the
create's transaction info as
+ // the OM's flushed marker. Sanity: the snapshot's changes are now
reported flushed.
+ getOmMetadataManager().getTransactionInfoTable().put(TRANSACTION_INFO_KEY,
+ TransactionInfo.fromByteString(snapshotInfo.getLastTransactionInfo()));
+
assertTrue(OmSnapshotManager.areSnapshotChangesFlushedToDB(getOmMetadataManager(),
key),
+ "sanity: the create transaction is flushed");
+
+ // Delete the snapshot at transaction index 2. The change is applied to
the table cache only; the
+ // double buffer has NOT flushed it (the flushed marker still points at
the create transaction).
+ OMSnapshotDeleteRequest omSnapshotDeleteRequest =
+ doPreExecute(deleteSnapshotRequest(getVolumeName(), getBucketName(),
snapshotName));
+ omSnapshotDeleteRequest.validateAndUpdateCache(getOzoneManager(), 2L);
+ snapshotInfo = getOmMetadataManager().getSnapshotInfoTable().get(key);
+ assertEquals(SNAPSHOT_DELETED, snapshotInfo.getSnapshotStatus());
+
+ // Correct behavior: the deletion (index 2) is not durable yet, so the
snapshot's changes must not be
+ // reported as flushed. Current behavior: the deletion never stamps
lastTransactionInfo, the check
+ // compares the stale create-time stamp against the flushed marker and
returns true.
+
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");
Review Comment:
The assertion failure message says the snapshot is reported flushed because
`OMSnapshotDeleteRequest` does not stamp `lastTransactionInfo`, but the
production change in this PR adds that stamp. Updating the message will keep
failures interpretable and avoid encoding stale behavior in the test output.
##########
hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/snapshot/filter/TestReclaimableKeyFilter.java:
##########
@@ -315,4 +317,71 @@ public void
testExclusiveSizeCalculationWithNonReclaimableKey(int actualNumberOf
testReclaimableKeyFilter(volume, bucket, index, keyInfo, prevKeyInfo,
prevPrevKeyInfo1,
prevKeyInfo == null, size, replicatedSize);
}
+
+ /**
+ * Boundary of the flush-lag reclamation window: the purge of the last path
snapshot has been APPLIED
+ * (the in-memory chain is empty) but is NOT yet flushed, so the on-disk
snapshotInfoTable still holds the
+ * snapshot's row. Reclamation proceeding here is safe ONLY because the row
can never be ACTIVE on disk:
+ * SnapshotDeletingService submits a purge only after the snapshot's
deletion is flushed
+ * (shouldIgnoreSnapshot -> areSnapshotChangesFlushedToDB, which relies on
OMSnapshotDeleteRequest stamping
+ * lastTransactionInfo). A DELETED-on-disk row is not user-readable, and
re-processing it after a
+ * restore-from-backup re-runs moveTableKeys/purge idempotently. If a
chain-removal path that bypasses that
+ * gate is ever added, this assumption breaks and reclamation here would
resurrect an ACTIVE snapshot with
+ * physically deleted blocks.
+ */
+ @Test
+ public void
testKeyReclaimableWhenChainEmptyingPurgeUnflushedButDeleteFlushed()
+ throws IOException, RocksDBException {
+ setup(2, 1, 1, 1, 1);
+ String volume = getVolumes().get(0);
+ String bucket = getBuckets().get(0);
+ SnapshotInfo purgedSnapshot = getSnapshotInfos().get(getKey(volume,
bucket)).get(0);
+
purgedSnapshot.setSnapshotStatus(SnapshotInfo.SnapshotStatus.SNAPSHOT_DELETED);
+
+ // SnapshotPurge has been applied: the in-memory chain no longer has the
snapshot...
+ getSnapshotInfos().get(getKey(volume, bucket)).clear();
+
+ // ...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.
Review Comment:
These comments imply the test is exercising a cache-vs-skip-cache
snapshotInfoTable difference, but `ReclaimableKeyFilter` determines emptiness
via `SnapshotChainManager.getLatestPathSnapshotId(...)` and doesn't consult
snapshotInfoTable when the chain is empty. Rewording the comment to clarify
that the table stubs are documenting an invariant (rather than being part of
the behavior under test) will avoid confusion for future maintainers.
--
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]
