-
Notifications
You must be signed in to change notification settings - Fork 588
HDDS-6646. Intermittent failure in TestOzoneFileSystem#testRenameToTrashEnabled #5217
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
2fcee89
d835998
b1b22f0
29e6c25
429eba3
768c833
fbc62a2
bfea174
02a62c5
183536e
900a80d
841d72d
fc4c53c
ec783f0
858e1ba
4ec8886
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -802,20 +802,12 @@ protected void deleteRootDir() throws IOException, InterruptedException { | |
| return; | ||
| } | ||
| deleteRootRecursively(fileStatuses); | ||
|
|
||
| // Waiting for double buffer flush before calling listStatus() again | ||
| // seem to have mitigated the flakiness in cleanup(), but at the cost of | ||
| // almost doubling the test run time. M1 154s->283s (all 4 sets of params) | ||
| cluster.getOzoneManager().awaitDoubleBufferFlush(); | ||
| // TODO: Investigate whether listStatus() is correctly iterating cache. | ||
|
|
||
| fileStatuses = fs.listStatus(ROOT); | ||
| if (fileStatuses != null) { | ||
| for (FileStatus fileStatus : fileStatuses) { | ||
| LOG.error("Unexpected file, should have been deleted: {}", fileStatus); | ||
| } | ||
| Assert.assertEquals( | ||
| "Delete root failed!", 0, fileStatuses.length); | ||
| Assert.assertEquals("Delete root failed!", 0, fileStatuses.length); | ||
| } | ||
| } | ||
|
|
||
|
|
@@ -1623,35 +1615,6 @@ public void testGetTrashRoots() throws IOException { | |
| Assert.assertEquals(6, res.size()); | ||
| } | ||
|
|
||
| /** | ||
| * Check that files are moved to trash. | ||
| * since fs.rename(src,dst,options) is enabled. | ||
| */ | ||
| @Test | ||
| @Flaky("HDDS-6646") | ||
| public void testRenameToTrashEnabled() throws Exception { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Was the test removal intended? because earlier commit had fixed the test and description also says so.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Yes @sadanand48 , as HDDS-6645 will be taking care of testTrash test case fix, so the only assertion which was different from
smengcl marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| // Create a file | ||
| String testKeyName = "testKey1"; | ||
| Path path = new Path(OZONE_URI_DELIMITER, testKeyName); | ||
| try (FSDataOutputStream stream = fs.create(path)) { | ||
| stream.write(1); | ||
| } | ||
|
|
||
| // Call moveToTrash. We can't call protected fs.rename() directly | ||
| trash.moveToTrash(path); | ||
|
|
||
| // Construct paths | ||
| String username = UserGroupInformation.getCurrentUser().getShortUserName(); | ||
| Path userTrash = new Path(TRASH_ROOT, username); | ||
| Path userTrashCurrent = new Path(userTrash, "Current"); | ||
| Path trashPath = new Path(userTrashCurrent, testKeyName); | ||
|
|
||
| // Trash Current directory should still have been created. | ||
| Assert.assertTrue(o3fs.exists(userTrashCurrent)); | ||
| // Check under trash, the key should be present | ||
| Assert.assertTrue(o3fs.exists(trashPath)); | ||
| } | ||
|
|
||
| /** | ||
| * 1.Move a Key to Trash | ||
| * 2.Verify that the key gets deleted by the trash emptier. | ||
|
|
@@ -1667,14 +1630,22 @@ public void testTrash() throws Exception { | |
| isAssignableFrom(TrashPolicyOzone.class)); | ||
| assertEquals(TRASH_INTERVAL, trash.getConf(). | ||
| getFloat(OMConfigKeys.OZONE_FS_TRASH_INTERVAL_KEY, 0), 0); | ||
| // Call moveToTrash. We can't call protected fs.rename() directly | ||
| trash.moveToTrash(path); | ||
|
|
||
| // Construct paths | ||
| String username = UserGroupInformation.getCurrentUser().getShortUserName(); | ||
| Path userTrash = new Path(TRASH_ROOT, username); | ||
| Path userTrashCurrent = new Path(userTrash, "Current"); | ||
| Path trashPath = new Path(userTrashCurrent, testKeyName); | ||
| Assert.assertFalse(o3fs.exists(userTrash)); | ||
|
|
||
| // Call moveToTrash. We can't call protected fs.rename() directly | ||
| trash.moveToTrash(path); | ||
| // Added this assertion here and will be tested as part of testTrash | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not following this comment. What do you mean by
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
As I mentioned, it will be handled as part of 6545 JIRA, and @sadanand48 will be working on it. Had a discussion on reason of flakiness and need to handle separately in testTrash |
||
| // test case which needs to be tested with separate mini cluster having | ||
| // emptier thread started with close match of timings of relevant | ||
| // assertion statements and corresponding trash and checkpoint interval. | ||
| Assert.assertTrue(o3fs.exists(userTrash)); | ||
| Assert.assertTrue(o3fs.exists(userTrashCurrent)); | ||
|
|
||
| // Wait until the TrashEmptier purges the key | ||
| GenericTestUtils.waitFor(() -> { | ||
|
|
@@ -1685,7 +1656,7 @@ public void testTrash() throws Exception { | |
| Assert.fail("Delete from Trash Failed"); | ||
| return false; | ||
| } | ||
| }, 1000, 120000); | ||
| }, 100, 120000); | ||
|
|
||
| // userTrash path will contain the checkpoint folder | ||
| FileStatus[] statusList = fs.listStatus(userTrash); | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure the removal of this line belongs to this PR?
#5244 still looks required.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes @smengcl #5244 needs anyway. I think while resolving conflict, it got removed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok. 5 out of 9 commits merged to master branch after this PR starts to have flaky
integration (filesystem)again as a result of this removal. Need to merge #5244 asap.https://github.com/apache/ozone/actions/runs/6100351514/job/16554482439
https://github.com/apache/ozone/actions/runs/6105400581/job/16568864195
https://github.com/apache/ozone/actions/runs/6107352731/job/16574193577
https://github.com/apache/ozone/actions/runs/6110706683/job/16584346159
https://github.com/apache/ozone/actions/runs/6114248488/job/16595437359