Copilot commented on code in PR #10726:
URL: https://github.com/apache/ozone/pull/10726#discussion_r3573251566
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/om/service/TestDirectoryDeletingServiceWithFSO.java:
##########
@@ -823,6 +809,28 @@ static void assertSubPathsCount(LongSupplier pathCount,
long expectedCount)
1000, 120000);
}
+ /**
+ * Wait until SnapshotDeletingService has purged snapshots and
snapshotInfoTable
+ * reaches the expected row count.
+ */
+ private void waitForSnapshotsPurged(Table<String, SnapshotInfo>
snapshotInfoTable,
+ int expectedCount) throws Exception {
+ SnapshotDeletingService snapshotDeletingService =
+ cluster.getOzoneManager().getKeyManager().getSnapshotDeletingService();
+ snapshotDeletingService.resume();
+ GenericTestUtils.waitFor(() -> {
+ try {
+ snapshotDeletingService.runPeriodicalTaskNow();
+ cluster.getOzoneManager().awaitDoubleBufferFlush();
+ return cluster.getOzoneManager().getMetadataManager()
+ .countRowsInTable(snapshotInfoTable) == expectedCount;
+} catch (Exception e) {
+ throw new RuntimeException("Failed to run SnapshotDeletingService purge
task", e);
+}
+ }, 1000, 120000);
+ assertTableRowCount(snapshotInfoTable, expectedCount);
+ }
Review Comment:
The new waitForSnapshotsPurged() helper has mis-indented try/catch braces
inside the lambda, which is likely to violate checkstyle and reduces
readability. It also calls assertTableRowCount() after a waitFor() that already
waits for the exact same condition, potentially doubling the wait time (up to 4
minutes) on slow runs.
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/om/service/TestDirectoryDeletingServiceWithFSO.java:
##########
@@ -571,7 +571,7 @@ public void
testAOSKeyDeletingWithSnapshotCreateParallelExecution()
Table<String, SnapshotInfo> snapshotInfoTable =
omMetadataManager.getSnapshotInfoTable();
Table<String, OmKeyInfo> deletedDirTable =
omMetadataManager.getDeletedDirTable();
Table<String, String> renameTable =
omMetadataManager.getSnapshotRenamedTable();
-
cluster.getOzoneManager().getKeyManager().getSnapshotDeletingService().shutdown();
+
cluster.getOzoneManager().getKeyManager().getSnapshotDeletingService().suspend();
DirectoryDeletingService dirDeletingService =
cluster.getOzoneManager().getKeyManager().getDirDeletingService();
// Suspend KeyDeletingService
dirDeletingService.suspend();
Review Comment:
The test suspends SnapshotDeletingService (and DirectoryDeletingService) but
only resumes it on the happy path. If the test fails before the resume call,
the shared mini-cluster can be left with SnapshotDeletingService suspended,
potentially cascading failures into later tests in this class/module. Consider
wrapping the body after suspending in a try/finally that resumes both services.
--
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]
