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

devmadhuu · 2023-08-24T12:01:01Z

What changes were proposed in this pull request?

This test case was flaky with listStatus issue which got resolved as part of HDDS-9041

Following points taken care as part of this PR:

testRenameToTrashEnabled : This test has been removed in this PR, however assertions of this test will be handled in testTrash as part of HDDS-6645
testTrash : this patch improves it, but still marked flaky as there is still a possibility of failure in some cases. will be handled as part of HDDS-6645
deleteRootDir cleanup method has removed awaitDoubleBuffer call though removal was not intended as part of this PR, however failure of assertion in deleteRootDir was due to the issue in listStatus API which is being fixed as part of this HDDS-9233. listStatus() API is not correctly iterating cache. #5244.

What is the link to the Apache JIRA

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

How was this patch tested?

This patch was tested with 10 times job runs with existing test cases.

…ashEnabled.

hemantk-12

Thanks @devmadhuu for fixing it.

Can you please also add how this patch would fix the issue in the PR description?

hemantk-12 · 2023-08-25T18:30:31Z

hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/fs/ozone/TestOzoneFileSystem.java

      return;
    }
    deleteRootRecursively(fileStatuses);
+    Thread.sleep(500);


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

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.

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);

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

devmadhuu · 2023-08-26T11:47:30Z

Thanks @devmadhuu for fixing it.

Can you please also add how this patch would fix the issue in the PR description?

This patch directly alone not contributing the resolution of this issue, however another patch which has fixed issue in listStatus API fixed the root cause as this test also was showing intermittent fail in deleteRootDir only, but when running multiple times in multiple jobs, this looks to be a race condition when tests running in parallel. And after adding sleep, I can see list status showing correct results.

devmadhuu · 2023-08-26T11:49:34Z

@sumitagrawl Pls review.

hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/fs/ozone/TestOzoneFileSystem.java

sumitagrawl

@devmadhuu thanks for working over this, have few query over the changes.

sumitagrawl · 2023-08-28T15:47:29Z

hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/fs/ozone/TestOzoneFileSystem.java

      return;
    }
    deleteRootRecursively(fileStatuses);
+    Thread.sleep(500);


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

+1

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

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.

hemantk-12 · 2023-08-28T22:42:33Z

Thanks @devmadhuu for fixing it.
Can you please also add how this patch would fix the issue in the PR description?

This patch directly alone not contributing the resolution of this issue, however another patch which has fixed issue in listStatus API fixed the root cause as this test also was showing intermittent fail in deleteRootDir only, but when running multiple times in multiple jobs, this looks to be a race condition when tests running in parallel. And after adding sleep, I can see list status showing correct results.

Can you please add the same in the description?

hemantk-12 · 2023-08-28T22:44:24Z

hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/fs/ozone/TestOzoneFileSystem.java

-    Thread.sleep(500);
    fileStatuses = fs.listStatus(ROOT);
+    FileStatus[] finalFileStatuses = fileStatuses;
+    GenericTestUtils.waitFor(


You are supposed to fetch fs.listStatus(ROOT); inside the lambda.

#5217 (comment)

Thanks @hemantk-12 , Its handled as per your suggestion. Kindly re-review.

devmadhuu · 2023-08-29T15:56:17Z

@sumitagrawl @hemantk-12 @sadanand48 Kindly re-review.

sadanand48 · 2023-08-29T16:49:47Z

Thanks @devmadhuu , patch looks good to me, IIUC, there are 3 tests in question:

testRenameToTrashEnabled : which the current patch fixes.
testTrash : this patch improves it, but still marked flaky as there is still a possibility of failure in some cases. will be handled as part of HDDS-6645
testListStatusOnLargeDirectory : which by fixing deleteRoot by adding a wait should stabilise the test further.

hemantk-12

Thanks for addressing review comments.

Overall changes are fine.

Few doubts and suggestions:

PR title is incorrect I think. It is supposed to be HDDS-6646 Intermittent failure ....
Can you please share the workflow run where you run the test 10 times? I found this, but in last iteration test still failed. Also I would suggest to run only testRenameToTrashEnabled and have a green CI for it.
I think deleteRootDir is not needed here because it is deleted in @After. If you agree, remove this either as part of this PR or may be separate PR.

hemantk-12 · 2023-08-29T17:01:20Z

hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/fs/ozone/TestOzoneFileSystem.java

+    // We can safely assert only trash directory here.
+    // Asserting Current or checkpoint directory is not feasible here in this
+    // test due to independent TrashEmptier thread running in cluster and
+    // possible flakyness is hard to avoid unless we test this test case


Suggested change

// possible flakyness is hard to avoid unless we test this test case

// possible flakiness is hard to avoid unless we test this test case

hemantk-12 · 2023-08-29T17:05:10Z

hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/fs/ozone/TestOzoneFileSystem.java


+    // 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


I'm not following this comment. What do you mean by will be tested as part of testTrash? This is testTrash test. Is there another testTrash? What am I missing?

I'm not following this comment. What do you mean by will be tested as part of testTrash? This is testTrash test. Is there another testTrash? What am I missing?

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

devmadhuu · 2023-08-29T17:37:20Z

Thanks for addressing review comments.

Overall changes are fine.

Few doubts and suggestions:

PR title is incorrect I think. It is supposed to be HDDS-6646 Intermittent failure ....

Can you please share the workflow run where you run the test 10 times? I found this, but in last iteration test still failed. Also I would suggest to run only testRenameToTrashEnabled and have a green CI for it.

I think deleteRootDir is not needed here because it is deleted in @After. If you agree, remove this either as part of this PR or may be separate PR.

@hemantk-12 , we can change PR title, however it's better to keep with JIRA title.

I'll re-run the test in multiple runs once again and get green CI.

I think we should keep deleteRootDir change as i observed that without this change, CI will not pass. Many other PR are blocked because of this and @after annotation makes it call after every test case including testRenameToTrashEnabled

hemantk-12 · 2023-08-29T18:14:48Z

Thanks for addressing review comments.
Overall changes are fine.
Few doubts and suggestions:

PR title is incorrect I think. It is supposed to be HDDS-6646 Intermittent failure ....

Can you please share the workflow run where you run the test 10 times? I found this, but in last iteration test still failed. Also I would suggest to run only testRenameToTrashEnabled and have a green CI for it.

I think deleteRootDir is not needed here because it is deleted in @After. If you agree, remove this either as part of this PR or may be separate PR.

@hemantk-12 , we can change PR title, however it's better to keep with JIRA title.

I'll re-run the test in multiple runs once again and get green CI.

I think we should keep deleteRootDir change as i observed that without this change, CI will not pass. Many other PR are blocked because of this

I didn't mean to remove Jira ID or Jira Title. ... just mean the continuation. I meant HDDS-6646. Intermittent failure in TestOzoneFileSystem#testRenameToTrashEnabled not Hdds 6646 - Intermittent failure in TestOzoneFileSystem#testRenameToTrashEnabled. PR title is supposed to start with HDDS-JiraID. followed by PR or jIra title
I am curious how deleteRootDir() calling solves the issue in test testListStatusOnLargeDirectory. Did you try to remove it and run? Do you have a run where it failed after it was removed?

devmadhuu · 2023-08-30T03:51:47Z

Thanks for addressing review comments.
Overall changes are fine.
Few doubts and suggestions:

PR title is incorrect I think. It is supposed to be HDDS-6646 Intermittent failure ....

Can you please share the workflow run where you run the test 10 times? I found this, but in last iteration test still failed. Also I would suggest to run only testRenameToTrashEnabled and have a green CI for it.

I think deleteRootDir is not needed here because it is deleted in @After. If you agree, remove this either as part of this PR or may be separate PR.

@hemantk-12 , we can change PR title, however it's better to keep with JIRA title.
I'll re-run the test in multiple runs once again and get green CI.
I think we should keep deleteRootDir change as i observed that without this change, CI will not pass. Many other PR are blocked because of this

I didn't mean to remove Jira ID or Jira Title. ... just mean the continuation. I meant HDDS-6646. Intermittent failure in TestOzoneFileSystem#testRenameToTrashEnabled not Hdds 6646 - Intermittent failure in TestOzoneFileSystem#testRenameToTrashEnabled. PR title is supposed to start with HDDS-JiraID. followed by PR or jIra title

I am curious how deleteRootDir() calling solves the issue in test testListStatusOnLargeDirectory. Did you try to remove it and run? Do you have a run where it failed after it was removed?

I fixed the PR title. Thanks for pointing out.
testListStatusOnLargeDirectory test was failed like any other test due to deleteRootDIr cleanup. No changes being done except deleteRootDir, May be we can remove from PR description about this test. I have updated the changes in PR description. Pls have a look.
And here is the link for successful and green CI multiple job runs for only TestOzoneFileSystem#testRenameToTrashEnabled case

devmadhuu · 2023-08-30T14:13:49Z

Here is the successful green CI from fork run in fork.

hemantk-12

Thanks for the patch @devmadhuu.

Overall looks good to me.

deleteRootDir is not needed in testListStatusOnLargeDirectory because it is deleted in @After.

I still believe there is no need to call deleteRootDir in testListStatusOnLargeDirectory. Because it is not expected from HDDS-6646, it can be handle as a separate task.

sumitagrawl

LGTM +1

sadanand48 · 2023-09-06T10:58:02Z

hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/fs/ozone/TestOzoneFileSystem.java

-   */
-  @Test
-  @Flaky("HDDS-6646")
-  public void testRenameToTrashEnabled() throws Exception {


Was the test removal intended? because earlier commit had fixed the test and description also says so.

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

smengcl · 2023-09-06T18:03:38Z

hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/fs/ozone/TestOzoneFileSystem.java

-    // 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();


I'm not sure the removal of this line belongs to this PR?

#5244 still looks required.

Yes @smengcl #5244 needs anyway. I think while resolving conflict, it got removed.

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

hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/fs/ozone/TestOzoneFileSystem.java

deveshsingh added 6 commits August 22, 2023 18:14

HDDS-6646. Intermittent failure in TestOzoneFileSystem#testRenameToTr…

2fcee89

…ashEnabled.

HDDS-6646. Intermittent failure in TestOzoneFileSystem#testRenameToTr…

d835998

…ashEnabled.

Merge remote-tracking branch 'origin/master' into HDDS-6646

b1b22f0

HDDS-6646. Intermittent failure in TestOzoneFileSystem#testRenameToTr…

29e6c25

…ashEnabled.

HDDS-6646. Intermittent failure in TestOzoneFileSystem#testRenameToTr…

429eba3

…ashEnabled.

HDDS-6646. Intermittent failure in TestOzoneFileSystem#testRenameToTr…

768c833

…ashEnabled.

hemantk-12 reviewed Aug 25, 2023

View reviewed changes

HDDS-6646. Fixing review comments.

fbc62a2

HDDS-6646. Fixing review comments.

bfea174

devmadhuu marked this pull request as ready for review August 26, 2023 11:49

sadanand48 reviewed Aug 28, 2023

View reviewed changes

hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/fs/ozone/TestOzoneFileSystem.java Outdated Show resolved Hide resolved

sumitagrawl reviewed Aug 28, 2023

View reviewed changes

hemantk-12 reviewed Aug 28, 2023

View reviewed changes

devmadhuu marked this pull request as draft August 29, 2023 13:00

devmadhuu marked this pull request as ready for review August 29, 2023 15:56

HDDS-6646. Fixing review comments.

02a62c5

hemantk-12 reviewed Aug 29, 2023

View reviewed changes

devmadhuu marked this pull request as draft August 29, 2023 18:14

deveshsingh added 2 commits August 29, 2023 23:45

HDDS-6646. Fixing review comments.

183536e

HDDS-6646. Fixing review comments.

900a80d

devmadhuu changed the title ~~Hdds 6646 - Intermittent failure in TestOzoneFileSystem#testRenameToTrashEnabled~~ HDDS-6646 - Intermittent failure in TestOzoneFileSystem#testRenameToTrashEnabled Aug 30, 2023

devmadhuu changed the title ~~HDDS-6646 - Intermittent failure in TestOzoneFileSystem#testRenameToTrashEnabled~~ HDDS-6646. Intermittent failure in TestOzoneFileSystem#testRenameToTrashEnabled Aug 30, 2023

devmadhuu mentioned this pull request Aug 30, 2023

HDDS-1796. Add admin access check for write opertaions in SCMClientProtocolServer and SCMBlockProtocolServer. #5203

Merged

devmadhuu marked this pull request as ready for review August 30, 2023 04:02

siddhantsangwan mentioned this pull request Aug 30, 2023

HDDS-5161. Fix the regex for key name validation #5200

Merged

ChenSammi mentioned this pull request Aug 30, 2023

HDDS-9217. Refine certificate renewer service to avoid it scheduled ahead of time #5227

Merged

deveshsingh added 2 commits August 30, 2023 17:29

HDDS-6646. Fixing review comments.

841d72d

HDDS-6646. Fixing review comments.

fc4c53c

hemantk-12 approved these changes Aug 30, 2023

View reviewed changes

This was referenced Aug 30, 2023

HDDS-8437. [FSO] S3A compatibility - dfs -mkdir creates a zero byte file instead of a directory #5124

Merged

HDDS-7275. Improve error logging when blocks cannot be allocated with RATIS THREE. #5189

Merged

deveshsingh added 3 commits September 5, 2023 16:45

HDDS-6646. Removed wait from deleteRootDir as not needed.

ec783f0

HDDS-6646. Fixing merge conflicts in deleteRootDir.

858e1ba

HDDS-6646. Fixing merge conflicts in deleteRootDir.

4ec8886

sumitagrawl approved these changes Sep 6, 2023

View reviewed changes

sadanand48 reviewed Sep 6, 2023

View reviewed changes

sumitagrawl merged commit 6657d0c into apache:master Sep 6, 2023

smengcl reviewed Sep 6, 2023

View reviewed changes

hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/fs/ozone/TestOzoneFileSystem.java Show resolved Hide resolved

	// possible flakyness is hard to avoid unless we test this test case
	// possible flakiness is hard to avoid unless we test this test case

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

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

Uh oh!

Conversation

devmadhuu commented Aug 24, 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!

hemantk-12 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hemantk-12 Aug 28, 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!

Choose a reason for hiding this comment

Uh oh!

devmadhuu commented Aug 26, 2023

Uh oh!

devmadhuu commented Aug 26, 2023

Uh oh!

Uh oh!

sumitagrawl left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hemantk-12 Aug 28, 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!

Choose a reason for hiding this comment

Uh oh!

hemantk-12 commented Aug 28, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

devmadhuu commented Aug 29, 2023

Uh oh!

sadanand48 commented Aug 29, 2023

Uh oh!

hemantk-12 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

devmadhuu Aug 29, 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 commented Aug 29, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hemantk-12 commented Aug 29, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

devmadhuu commented Aug 30, 2023

Uh oh!

hemantk-12 left a comment

Choose a reason for hiding this comment

Uh oh!

sumitagrawl left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

devmadhuu commented Aug 24, 2023 •

edited

Loading

hemantk-12 Aug 28, 2023 •

edited

Loading

hemantk-12 Aug 28, 2023 •

edited

Loading

devmadhuu Aug 29, 2023 •

edited

Loading

devmadhuu commented Aug 29, 2023 •

edited

Loading

hemantk-12 commented Aug 29, 2023 •

edited

Loading

devmadhuu commented Aug 30, 2023 •

edited

Loading