HDDS-8877. Intermittent failure in TestOzoneFileSystem#testListStatusOnKeyNameContainDelimiter #5093

chungen0126 · 2023-07-20T11:49:32Z

What changes were proposed in this pull request?

When the fileSystemPaths isn't enable, listing status from cache will skip the keys with slash. To fix it, we need to create fakeDirs.

What is the link to the Apache JIRA

HDDS-8877

Please replace this section with the link to the Apache JIRA)

How was this patch tested?

Succeeded in all 10 * 10 run for TestOzoneFileSystem.
https://github.com/chungen0126/ozone/actions/runs/6081955837

jojochuang

Overall looks fine. unit tests passed so I am +1. Some nits that's more cosmetic in nature.
Re-triggered the test to check the acceptance tests.

hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/KeyManagerImpl.java

devmadhuu · 2023-09-05T10:58:01Z

hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/KeyManagerImpl.java

          if (key.startsWith(targetKey)) {
            if (!Objects.equals(key, targetKey)
-                && !isKeyDeleted(key, keyTable)) {
+                && keyTable.isExist(key)) {


@chungen0126 Thanks for working on this patch, however instead of double checking , let deleted key flow outside of this method to caller and outside , it can be filtered. I think the issue is due to the cache is getting cleared after delete operation and isKeyDeleted will not tell as expected. Pls check my patch here. We may need not this extra call of "keyTable.isExist(key)"

@devmadhuu Thanks for your review. Here is an issue about that creating a deleteSet will lead to bloat in memory. I'm not sure if it is bothered now.

I wonder if it is fine to add deleted keys to TreeMap. Once It is resolved by the PR, I will remove my changes about listing keys from DB.

Thanks @chungen0126 @devmadhuu .

Should we consider this PR superceded by #5244 ? Since the latter is fixing the same issue in a cleaner way

I am not sure if isExists() API for checking the existence of key till it finds target key will impact performance, as cache is always faster. I applied the patch and ran multiple iterations of each job run in CI for single test class TestOzoneFileSystem and I can notice the time taken in completing the test is slightly increased. Also I can notice multiple test run in CI still failing.

devmadhuu · 2023-09-09T09:46:02Z

hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/KeyManagerImpl.java

          if (key.startsWith(targetKey)) {
            if (!Objects.equals(key, targetKey)
-                && !isKeyDeleted(key, keyTable)) {
+                && keyTable.isExist(key)) {


I am not sure if isExists() API for checking the existence of key till it finds target key will impact performance, as cache is always faster. I applied the patch and ran multiple iterations of each job run in CI for single test class TestOzoneFileSystem and I can notice the time taken in completing the test is slightly increased. Also I can notice multiple test run in CI still failing.

devmadhuu · 2023-09-09T09:48:56Z

hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OzoneListStatusHelper.java

        String entryKey = entry.getKey();
        if (entryKey.startsWith(prefixKey)) {
-          if (!KeyManagerImpl.isKeyDeleted(entryKey, table)) {
+          if (table.isExist(entryKey)) {


getNextKey() call is on every hasNextCall(), so again here we are iterating on table and checking existence of key, so is it acceptable from performance point of view ? One more point that as per isExist API doc:

A lock on the key / bucket needs to be acquired before invoking this API.

jojochuang · 2023-09-19T18:07:56Z

@chungen0126 if you can confirm #5244 fixes the same bug we can close it. Thank you.

adoroszlai · 2023-09-21T10:40:51Z

@chungen0126 if you can confirm #5244 fixes the same bug we can close it. Thank you.

I guess this is a different problem, as it has also happened after the other fix was merged:

https://github.com/adoroszlai/ozone-build-results/blob/master/2023/09/19/25476/it-filesystem/hadoop-ozone/integration-test/org.apache.hadoop.fs.ozone.TestOzoneFileSystem.txt

devmadhuu · 2023-09-21T11:06:12Z

@chungen0126 if you can confirm #5244 fixes the same bug we can close it. Thank you.

I guess this is a different problem, as it has also happened after the other fix was merged:

https://github.com/adoroszlai/ozone-build-results/blob/master/2023/09/19/25476/it-filesystem/hadoop-ozone/integration-test/org.apache.hadoop.fs.ozone.TestOzoneFileSystem.txt

I think the other issue also were handled in PR #5252 , but still open

szetszwo · 2023-09-25T18:28:41Z

Is this good to merge? If not, could we mark TestOzoneFileSystem.testListStatusOnKeyNameContainDelimiter as @Flaky? It is failing quite often, e.g. https://github.com/apache/ozone/actions/runs/6285259623/job/17068827983?pr=5338#step:5:3769

adoroszlai · 2023-09-25T18:38:50Z

could we mark TestOzoneFileSystem.testListStatusOnKeyNameContainDelimiter as @Flaky?

Unfortunately these filesystem tests are implemented using JUnit4, but the @Flaky annotation applies only to JUnit5 tests.

Also, the test itself is probably OK, fix is proposed for production code.

chungen0126 · 2023-09-25T18:58:41Z

This PR fixs the error of listing status from cache, not the race condition in listStatus. It might be different bug in PR #5252.

In master, listing status from cache only skip the keys with slash. To fix it, I create some fakeDirs for some condition if enableFileSystemPaths was false. When enableFileSystemPaths is false and I create a key with slash called "dir1/dir2/key1", there will be a key called "dir1/dir2/key1" in table.
Before fixs, If I listed "/", it skip the key "dir1/dir2/key1" and return nothing.
After fixs, If I listed "/", it creates a fakeDir "/dir1/" to be returned not just skip it.

I also make the code more clear in listStatus, and I think it is probably OK now.

jojochuang · 2023-10-06T01:18:08Z

The fix makes sense to me. I'm curious why it is intermittent though.
There is no test so hard to confirm it does fix the problem. Also, what if there's another layer of directory?

adoroszlai · 2023-10-15T16:57:52Z

@chungen0126 @jojochuang It seems #5399 also fixes the same problem and more. Can you please review that one?

adoroszlai · 2023-10-20T08:34:23Z

Thanks a lot @chungen0126 for the patch. The bug is now fixed in HDDS-9347, so I'm closing this one. However, to give credit to you for your work on this PR, I have added you as co-author of 84fb0b4.

Also thanks @devmadhuu, @jojochuang for the review.

chungen0126 added 2 commits July 19, 2023 20:34

fix listStatus in table cache.

5070474

fix list status in table cache with recursive

38bab15

umamaheswararao requested a review from jojochuang July 24, 2023 16:31

jojochuang approved these changes Jul 24, 2023

View reviewed changes

chungen0126 added 4 commits July 28, 2023 08:08

add javadoc

65b7845

Merge branch 'master' into HDDS-8877

cbd4ddf

fix consistent between cache and DB

205b2b4

clean up isKeyDeleted

1cd77af

devmadhuu reviewed Sep 5, 2023

View reviewed changes

chungen0126 requested a review from devmadhuu September 5, 2023 13:36

smengcl requested review from smengcl and removed request for devmadhuu September 5, 2023 18:20

smengcl mentioned this pull request Sep 8, 2023

HDDS-9233. listStatus() API is not correctly iterating cache. #5244

Merged

smengcl requested a review from devmadhuu September 8, 2023 22:04

devmadhuu reviewed Sep 9, 2023

View reviewed changes

devmadhuu mentioned this pull request Sep 9, 2023

HDDS-7941. Race condition in getFileStatus causes flaky testObjectStoreCreateWithO3fs #5252

Merged

fix conflict

230b930

ChenSammi mentioned this pull request Sep 22, 2023

HDDS-8869. Make DN DeleteBlocksCommandHandler wait for the lock can timeout. #4913

Merged

Merge branch 'master' into HDDS-8877

612fc3a

restore OzoneListStatusHelper

035157a

adoroszlai mentioned this pull request Oct 4, 2023

HDDS-9381. Remove @Flaky annotations from now-stable test cases. #5390

Merged

jojochuang mentioned this pull request Oct 16, 2023

HDDS-9347. Fix Ozone FS listStatus() cache-table inconsistencies. #5399

Merged

3 tasks

adoroszlai closed this Oct 20, 2023

HDDS-8877. Intermittent failure in TestOzoneFileSystem#testListStatusOnKeyNameContainDelimiter #5093

HDDS-8877. Intermittent failure in TestOzoneFileSystem#testListStatusOnKeyNameContainDelimiter #5093

Uh oh!

Conversation

chungen0126 commented Jul 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

What is the link to the Apache JIRA

How was this patch tested?

Uh oh!

jojochuang left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

devmadhuu Sep 5, 2023

Choose a reason for hiding this comment

Uh oh!

chungen0126 Sep 5, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chungen0126 Sep 5, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

smengcl Sep 6, 2023

Choose a reason for hiding this comment

Uh oh!

devmadhuu Sep 9, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

devmadhuu Sep 9, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

devmadhuu Sep 9, 2023

Choose a reason for hiding this comment

Uh oh!

jojochuang commented Sep 19, 2023

Uh oh!

adoroszlai commented Sep 21, 2023

Uh oh!

devmadhuu commented Sep 21, 2023

Uh oh!

szetszwo commented Sep 25, 2023

Uh oh!

adoroszlai commented Sep 25, 2023

Uh oh!

chungen0126 commented Sep 25, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jojochuang commented Oct 6, 2023

Uh oh!

adoroszlai commented Oct 15, 2023

Uh oh!

adoroszlai commented Oct 20, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

chungen0126 commented Jul 20, 2023 •

edited

Loading

chungen0126 Sep 5, 2023 •

edited

Loading

chungen0126 Sep 5, 2023 •

edited

Loading

devmadhuu Sep 9, 2023 •

edited

Loading

devmadhuu Sep 9, 2023 •

edited

Loading

chungen0126 commented Sep 25, 2023 •

edited

Loading