Copilot commented on code in PR #11212:
URL: https://github.com/apache/ozone/pull/11212#discussion_r4049700381
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/om/service/TestDirectoryDeletingServiceWithFSO.java:
##########
@@ -149,7 +156,36 @@ public static void teardown() {
}
@AfterEach
- public void cleanup() throws InterruptedException, TimeoutException {
+ public void cleanup() throws Exception {
+ try {
+ if (pendingSdsResume) {
+
cluster.getOzoneManager().getKeyManager().getSnapshotDeletingService().resume();
+ pendingSdsResume = false;
+ }
+ if (snapshotToDeleteInCleanup != null) {
+ try {
+ client.getObjectStore().deleteSnapshot(volumeName, bucketName,
snapshotToDeleteInCleanup);
+ } catch (Exception ignored) {
+ // snapshot may have already been deleted by the test body
+ }
+ snapshotToDeleteInCleanup = null;
+ }
+ if (snapshotCountAfterTest >= 0) {
+ Table<String, SnapshotInfo> snapshotInfoTable =
+
cluster.getOzoneManager().getMetadataManager().getSnapshotInfoTable();
+ waitForSnapshotsPurged(snapshotInfoTable, snapshotCountAfterTest);
+ snapshotCountAfterTest = -1;
+ }
+ } finally {
+ if (pendingResumeDds != null) {
+ pendingResumeDds.resume();
+ pendingResumeDds = null;
+ }
+ if (needsTableCleanup) {
+ cleanupTables();
+ needsTableCleanup = false;
+ }
Review Comment:
`cleanupTables()` currently runs from the `finally` block whenever
`needsTableCleanup` is true, even if `waitForSnapshotsPurged(...)` timed out or
snapshot deletion failed earlier in the `try` block. That can wipe
`directoryTable`/`fileTable` while snapshots still exist, potentially leaving
the shared OM metadata in a broken state for subsequent tests.
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/om/service/TestDirectoryDeletingServiceWithFSO.java:
##########
@@ -571,10 +607,12 @@ 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();
+ pendingSdsResume = true;
DirectoryDeletingService dirDeletingService =
cluster.getOzoneManager().getKeyManager().getDirDeletingService();
// Suspend KeyDeletingService
dirDeletingService.suspend();
+ pendingResumeDds = dirDeletingService;
Random random = new Random();
Review Comment:
`@AfterEach` cleanup can only wait for `snapshotInfoTable` to return to
`snapshotCountAfterTest`; it does not proactively delete the snapshots created
in this test (snap1/snap2) if an assertion fails before the end-of-test
`deleteSnapshot(...)` calls. In that failure mode, cleanup will time out and
the shared mini-cluster can still be left with leftover snapshots that affect
later tests.
This issue also appears on line 754 of the same file.
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/om/service/TestDirectoryDeletingServiceWithFSO.java:
##########
@@ -149,7 +156,36 @@ public static void teardown() {
}
@AfterEach
- public void cleanup() throws InterruptedException, TimeoutException {
+ public void cleanup() throws Exception {
+ try {
+ if (pendingSdsResume) {
+
cluster.getOzoneManager().getKeyManager().getSnapshotDeletingService().resume();
+ pendingSdsResume = false;
+ }
+ if (snapshotToDeleteInCleanup != null) {
+ try {
+ client.getObjectStore().deleteSnapshot(volumeName, bucketName,
snapshotToDeleteInCleanup);
+ } catch (Exception ignored) {
+ // snapshot may have already been deleted by the test body
+ }
Review Comment:
The cleanup path catches `Exception` here, but `ObjectStore#deleteSnapshot`
only declares `IOException`. Narrowing the catch avoids accidentally swallowing
unexpected runtime errors that could indicate a real test/environment problem.
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/om/service/TestDirectoryDeletingServiceWithFSO.java:
##########
@@ -107,6 +107,13 @@ public class TestDirectoryDeletingServiceWithFSO {
private static OzoneClient client;
private static DeletingServiceMetrics metrics;
+ // Per-test state for @AfterEach cleanup: tracked when a test suspends
services or needs table cleanup.
+ private int snapshotCountAfterTest = -1;
+ private boolean pendingSdsResume = false;
+ private DirectoryDeletingService pendingResumeDds = null;
+ private boolean needsTableCleanup = false;
+ private String snapshotToDeleteInCleanup = null;
Review Comment:
This PR also deletes
`hadoop-ozone/dist/src/main/compose/common/grafana/dashboards/Ozone - Container
Balancer Metrics.json`, but the PR description is scoped to fixing flakiness in
this test. Please confirm the dashboard deletion is intended; otherwise it
should be reverted or moved to a separate PR.
--
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]
