Skip to content

Conversation

@sadanand48
Copy link
Contributor

@sadanand48 sadanand48 commented Oct 6, 2023

What changes were proposed in this pull request?

Few bugs found and fixed while testing with cache unflushed

  • Fake dir not created when fsPaths is disabled
  • CacheIterators should always skip null values as when we delete an object we mark it as null in cache first and later delete it from the RawTable
  • StartKey means the starting key from which listStatus should display in order not that keyName should start with the key. This is fixed. The below current code has this bug.
if (cacheOmKeyInfo != null
          && cacheKey.startsWith(startCacheKey)
          && cacheKey.compareTo(startCacheKey) >= 0) {

What is the link to the Apache JIRA

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

How was this patch tested?

Added an extra combination in TestRootedOzoneFileSystem to test when the OM Table cache is not flushed, This is useful in catching hold of issues that is caused by cache-table inconsistencies

Ran the entire test with extra combinations too : 15x10 times here : https://github.com/sadanand48/hadoop-ozone/actions/runs/6415907619

@adoroszlai
Copy link
Contributor

@sadanand48 thanks for the patch. Can you please trigger repeated runs for a few more FS test classes (TestOzoneFileSystem, etc.) on this branch in your fork?

@adoroszlai
Copy link
Contributor

@adoroszlai adoroszlai requested a review from jojochuang October 12, 2023 14:38
@arp7
Copy link
Contributor

arp7 commented Oct 12, 2023

@swamirishi can you review this change?

@kerneltime
Copy link
Contributor

kerneltime commented Oct 16, 2023

@SaketaChalamchala @tanvipenumudy can you take a look at this as well?

@jojochuang
Copy link
Contributor

Looks very similar to the bug described in #5093 / HDDS-8877

Copy link
Contributor

@swamirishi swamirishi left a comment

Choose a reason for hiding this comment

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

Thanks for the patch @sadanand48. Overall the patch looks good to me. Have a minor nitpicky comment you can take care of.

OzoneFSUtils.getImmediateChild(remainingKey, keyArgs);
if (!cacheKeyMap.containsKey(immediateChild)) {
// immediateChild contains volume/bucket prefix remove it.
String volumeBuckPrefix = OZONE_URI_DELIMITER + entry.getValue()
Copy link
Contributor

Choose a reason for hiding this comment

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

from what i understand this list would be for only one bucket. This can be outside the loop, it would be a constant string for this function.

Copy link
Contributor

Choose a reason for hiding this comment

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

Can we move this to a follow-up task? It would be nice to avoid blocking the fix for a single nitpicky comment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @swamirishi for the review. Addressed the comment.

if (cacheOmKeyInfo != null
&& cacheKey.startsWith(startCacheKey)
&& cacheKey.compareTo(startCacheKey) >= 0) {
if (cacheOmKeyInfo != null && cacheKey.startsWith(
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a test that will fail without this fix?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, e.g. testListStatusIteratorOnPageSize fails intermittently with small page size.

https://github.com/apache/ozone/actions/runs/6263989725/job/17010674585#step:5:3786

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also the new test changes in this PR will fail without code changes

@adoroszlai adoroszlai requested a review from swamirishi October 19, 2023 15:34
@adoroszlai adoroszlai merged commit 84fb0b4 into apache:master Oct 20, 2023
@adoroszlai
Copy link
Contributor

adoroszlai commented Oct 20, 2023

Thanks @sadanand48 for the patch, @jojochuang, @kerneltime, @swamirishi for the review.

ibrusentsev pushed a commit to ibrusentsev/ozone that referenced this pull request Nov 14, 2023
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.

6 participants