Skip to content

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.

Sign up for GitHub

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

Merged
sumitagrawl merged 16 commits into from
Sep 6, 2023
Merged

HDDS-6646. Intermittent failure in TestOzoneFileSystem#testRenameToTrashEnabled #5217

Changes from 6 commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
2fcee89
HDDS-6646. Intermittent failure in TestOzoneFileSystem#testRenameToTr…
Aug 22, 2023
d835998
HDDS-6646. Intermittent failure in TestOzoneFileSystem#testRenameToTr…
Aug 24, 2023
b1b22f0
Merge remote-tracking branch 'origin/master' into HDDS-6646
Aug 24, 2023
29e6c25
HDDS-6646. Intermittent failure in TestOzoneFileSystem#testRenameToTr…
Aug 24, 2023
429eba3
HDDS-6646. Intermittent failure in TestOzoneFileSystem#testRenameToTr…
Aug 24, 2023
768c833
HDDS-6646. Intermittent failure in TestOzoneFileSystem#testRenameToTr…
Aug 24, 2023
fbc62a2
HDDS-6646. Fixing review comments.
Aug 26, 2023
bfea174
HDDS-6646. Fixing review comments.
Aug 26, 2023
02a62c5
HDDS-6646. Fixing review comments.
Aug 29, 2023
183536e
HDDS-6646. Fixing review comments.
Aug 29, 2023
900a80d
HDDS-6646. Fixing review comments.
Aug 30, 2023
841d72d
HDDS-6646. Fixing review comments.
Aug 30, 2023
fc4c53c
HDDS-6646. Fixing review comments.
Aug 30, 2023
ec783f0
HDDS-6646. Removed wait from deleteRootDir as not needed.
Sep 5, 2023
858e1ba
HDDS-6646. Fixing merge conflicts in deleteRootDir.
Sep 6, 2023
4ec8886
HDDS-6646. Fixing merge conflicts in deleteRootDir.
Sep 6, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 1 addition & 2 deletions ...-ozone/integration-test/src/test/java/org/apache/hadoop/fs/ozone/TestOzoneFileSystem.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -802,6 +802,7 @@ protected void deleteRootDir() throws IOException, InterruptedException {
return;
}
deleteRootRecursively(fileStatuses);
Thread.sleep(500);

@hemantk-12 hemantk-12 Aug 25, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we use GenericTestUtils#waitFor instead of Thread.sleep()?

@devmadhuu devmadhuu Aug 26, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we use GenericTestUtils#waitFor instead of Thread.sleep()?

@hemantk-12 thanks for the review, I tried using GenericTestUtils, however this in actual run throwing always TimeOutException and not working in this case.

@hemantk-12 hemantk-12 Aug 28, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm surprise that Thread.sleep() works fine but GenericTestUtils#waitFor doesn't. Just curious what was lambda used?

I feel this should work if it is just about cleaning dir.

    GenericTestUtils.waitFor(() -> {
      try {
        FileStatus[] fileStatus = fs.listStatus(ROOT);
        return fileStatus != null && fileStatus.length == 0;
      } catch (IOException e) {
       // You can decide if it should be false or throw RTE.
        return false;
      }
    }, 100, 500);

@devmadhuu devmadhuu Aug 29, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @hemantk-12 for review. I have handled as per your suggestion.

@sumitagrawl sumitagrawl Aug 28, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How sleep for 500ms solves the problem?
And Who throws TimeoutException in what case make it flaky ?

@hemantk-12 hemantk-12 Aug 28, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

I don't get either why TimeoutException is added here. May be @devmadhuu forgot to remove it while reverting some change.

@devmadhuu devmadhuu Aug 29, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since now GenericTestUtils is being used, I have not removed TimeoutException. This change is not directly related, however many other tests are getting impacted because of deleteRootDIr cleanup, so this is important as part of this test as well.

fileStatuses = fs.listStatus(ROOT);
if (fileStatuses != null) {
Assert.assertEquals(
Expand Down Expand Up @@ -1618,15 +1619,13 @@ public void testGetTrashRoots() throws IOException {
* since fs.rename(src,dst,options) is enabled.
*/
@Test
@Flaky("HDDS-6646")
public void testRenameToTrashEnabled() throws Exception {
Comment thread
sadanand48 marked this conversation as resolved.
Outdated

@sadanand48 sadanand48 Sep 6, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The 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.

@devmadhuu devmadhuu Sep 6, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The 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.

Yes @sadanand48 , as HDDS-6645 will be taking care of testTrash test case fix, so the only assertion which was different from testRenameToTrashEnabled and testTrash has been moved to testTrash and testRenameToTrashEnabled has been removed as per suggestion also from @sumitagrawl

Comment thread
smengcl marked this conversation as 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);

Expand Down