Skip to content

Conversation

@sadanand48
Copy link
Contributor

@sadanand48 sadanand48 commented Sep 20, 2023

What changes were proposed in this pull request?

  1. Modify the check in the test after the key moves to trash to to assert that the key is either in the Current or the checkpoint directory as the checkpoint could immediately happen in some cases.
  2. Make Trash FS Ozone Listing Iterator use omMetadataManager#listKeys instead of iterating the Raw key table directly as it won't account for keys in Cache. This solves cases where if Cache is not flushed after rename to trash or delete from trash.
  3. One of the main fixes for this is to fix createFakeDirIfShould to avoid race condition b/w cache flush and opening an older KeyTable Iterator reference solved by HDDS-7941. Race condition in getFileStatus causes flaky testObjectStoreCreateWithO3fs #5252

What is the link to the Apache JIRA

https://issues.apache.org/jira/browse/HDDS-6645

How was this patch tested?

Unit test
Ran the test successfully for 15x10 = 150 times here : https://github.com/sadanand48/hadoop-ozone/actions/runs/6304966964

@sadanand48 sadanand48 marked this pull request as ready for review September 25, 2023 06:49
@sadanand48
Copy link
Contributor Author

@devmadhuu @ashishkumar50 Could you please take a look?

@adoroszlai
Copy link
Contributor

Thanks @sadanand48 for the patch.

Ran the test successfully for 15x10 = 150 times here : https://github.com/sadanand48/hadoop-ozone/actions/runs/6295908918

One question: the patch changes TestOzoneFileSystem, but the run tested TestRootedOzoneFileSystem, is that intentional? Do we have repeated run for TestOzoneFileSystem, too?

@sadanand48
Copy link
Contributor Author

Do we have repeated run for TestOzoneFileSystem, too?

Thanks @adoroszlai for pointing. out, updated link for TestOzoneFileSystem run in description.

Copy link
Contributor

@ashishkumar50 ashishkumar50 left a comment

Choose a reason for hiding this comment

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

@sadanand48, Thanks for working on this. Please find few comments inline.

Copy link
Contributor

@ashishkumar50 ashishkumar50 left a comment

Choose a reason for hiding this comment

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

@sadanand48 Thanks for updating the patch, LGTM +1.

Copy link
Contributor

@aryangupta1998 aryangupta1998 left a comment

Choose a reason for hiding this comment

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

LGTM, +1
Thanks @sadanand48 for the patch and @ashishkumar50 for the review!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants