You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@ozone.apache.org by "devmadhuu (via GitHub)" <gi...@apache.org> on 2023/08/24 12:01:04 UTC
[GitHub] [ozone] devmadhuu opened a new pull request, #5217: Hdds 6646 - Intermittent failure in TestOzoneFileSystem#testRenameToTrashEnabled
devmadhuu opened a new pull request, #5217:
URL: https://github.com/apache/ozone/pull/5217
## What changes were proposed in this pull request?
This test case was flaky with listStatus issue which got resolved as part of [HDDS-9041](https://issues.apache.org/jira/browse/HDDS-9041)
## 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] sumitagrawl commented on a diff in pull request #5217: Hdds 6646 - Intermittent failure in TestOzoneFileSystem#testRenameToTrashEnabled
Posted by "sumitagrawl (via GitHub)" <gi...@apache.org>.
sumitagrawl commented on code in PR #5217:
URL: https://github.com/apache/ozone/pull/5217#discussion_r1307599898
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/fs/ozone/TestOzoneFileSystem.java:
##########
@@ -795,13 +796,15 @@ public void testListStatusOnKeyNameContainDelimiter() throws Exception {
*
* @throws IOException DB failure
*/
- protected void deleteRootDir() throws IOException, InterruptedException {
+ protected void deleteRootDir()
+ throws IOException, InterruptedException, TimeoutException {
FileStatus[] fileStatuses = fs.listStatus(ROOT);
if (fileStatuses == null) {
return;
}
deleteRootRecursively(fileStatuses);
+ Thread.sleep(500);
Review Comment:
How sleep for 500ms solves the problem?
And Who throws TimeoutException in what case make it flaky ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] devmadhuu commented on pull request #5217: Hdds 6646 - Intermittent failure in TestOzoneFileSystem#testRenameToTrashEnabled
Posted by "devmadhuu (via GitHub)" <gi...@apache.org>.
devmadhuu commented on PR #5217:
URL: https://github.com/apache/ozone/pull/5217#issuecomment-1694318786
@sumitagrawl Pls review.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] hemantk-12 commented on a diff in pull request #5217: HDDS-6646. Intermittent failure in TestOzoneFileSystem#testRenameToTrashEnabled
Posted by "hemantk-12 (via GitHub)" <gi...@apache.org>.
hemantk-12 commented on code in PR #5217:
URL: https://github.com/apache/ozone/pull/5217#discussion_r1317656737
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/fs/ozone/TestOzoneFileSystem.java:
##########
@@ -1623,35 +1615,6 @@ public void testGetTrashRoots() throws IOException {
Assert.assertEquals(6, res.size());
}
- /**
- * Check that files are moved to trash.
- * since fs.rename(src,dst,options) is enabled.
- */
- @Test
- @Flaky("HDDS-6646")
- public void testRenameToTrashEnabled() throws Exception {
Review Comment:
+1
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] devmadhuu commented on a diff in pull request #5217: Hdds 6646 - Intermittent failure in TestOzoneFileSystem#testRenameToTrashEnabled
Posted by "devmadhuu (via GitHub)" <gi...@apache.org>.
devmadhuu commented on code in PR #5217:
URL: https://github.com/apache/ozone/pull/5217#discussion_r1309045252
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/fs/ozone/TestOzoneFileSystem.java:
##########
@@ -795,13 +796,15 @@ public void testListStatusOnKeyNameContainDelimiter() throws Exception {
*
* @throws IOException DB failure
*/
- protected void deleteRootDir() throws IOException, InterruptedException {
+ protected void deleteRootDir()
+ throws IOException, InterruptedException, TimeoutException {
FileStatus[] fileStatuses = fs.listStatus(ROOT);
if (fileStatuses == null) {
return;
}
deleteRootRecursively(fileStatuses);
+ Thread.sleep(500);
Review Comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] devmadhuu commented on a diff in pull request #5217: Hdds 6646 - Intermittent failure in TestOzoneFileSystem#testRenameToTrashEnabled
Posted by "devmadhuu (via GitHub)" <gi...@apache.org>.
devmadhuu commented on code in PR #5217:
URL: https://github.com/apache/ozone/pull/5217#discussion_r1309042720
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/fs/ozone/TestOzoneFileSystem.java:
##########
@@ -802,6 +802,7 @@ protected void deleteRootDir() throws IOException, InterruptedException {
return;
}
deleteRootRecursively(fileStatuses);
+ Thread.sleep(500);
Review Comment:
Thanks @hemantk-12 for review. I have handled as per your suggestion.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] hemantk-12 commented on pull request #5217: Hdds 6646 - Intermittent failure in TestOzoneFileSystem#testRenameToTrashEnabled
Posted by "hemantk-12 (via GitHub)" <gi...@apache.org>.
hemantk-12 commented on PR #5217:
URL: https://github.com/apache/ozone/pull/5217#issuecomment-1697918124
> > Thanks for addressing review comments.
> > Overall changes are fine.
> > Few doubts and suggestions:
> >
> > 1. PR title is incorrect I think. It is supposed to be `HDDS-6646 Intermittent failure ....`
> > 2. Can you please share the workflow run where you run the test 10 times? I found [this](https://github.com/devmadhuu/ozone/actions/runs/5938962749/job/16109360981), but in last iteration test still failed. Also I would suggest to run only `testRenameToTrashEnabled` and have a green CI for it.
> > 3. I think [deleteRootDir](https://github.com/apache/ozone/blob/011de37b19ec874819ae5bbf726580ba5b0b7f93/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/fs/ozone/TestOzoneFileSystem.java#L721) is not needed here because it is deleted in [@After](https://github.com/apache/ozone/blob/011de37b19ec874819ae5bbf726580ba5b0b7f93/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/fs/ozone/TestOzoneFileSystem.java#L228). 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
1. I didn't mean to remove Jira ID or Jira Title. 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
2. I am curious how `deleteRootDir()` calling solves the issue in test [testListStatusOnLargeDirectory](https://github.com/apache/ozone/blob/011de37b19ec874819ae5bbf726580ba5b0b7f93/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/fs/ozone/TestOzoneFileSystem.java#L720). Did you try to remove it and run? Do you have a run where it failed after it was removed?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] smengcl commented on a diff in pull request #5217: HDDS-6646. Intermittent failure in TestOzoneFileSystem#testRenameToTrashEnabled
Posted by "smengcl (via GitHub)" <gi...@apache.org>.
smengcl commented on code in PR #5217:
URL: https://github.com/apache/ozone/pull/5217#discussion_r1317652233
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/fs/ozone/TestOzoneFileSystem.java:
##########
@@ -1623,35 +1615,6 @@ public void testGetTrashRoots() throws IOException {
Assert.assertEquals(6, res.size());
}
- /**
- * Check that files are moved to trash.
- * since fs.rename(src,dst,options) is enabled.
- */
- @Test
- @Flaky("HDDS-6646")
- public void testRenameToTrashEnabled() throws Exception {
Review Comment:
Would you update the PR description to reflect that `testRenameToTrashEnabled` is removed for now?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] devmadhuu commented on a diff in pull request #5217: Hdds 6646 - Intermittent failure in TestOzoneFileSystem#testRenameToTrashEnabled
Posted by "devmadhuu (via GitHub)" <gi...@apache.org>.
devmadhuu commented on code in PR #5217:
URL: https://github.com/apache/ozone/pull/5217#discussion_r1306364360
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/fs/ozone/TestOzoneFileSystem.java:
##########
@@ -802,6 +802,7 @@ protected void deleteRootDir() throws IOException, InterruptedException {
return;
}
deleteRootRecursively(fileStatuses);
+ Thread.sleep(500);
Review Comment:
> Can we use [GenericTestUtils#waitFor](https://github.com/apache/ozone/blob/387c53781aae03f944906a0add58f51250d36687/hadoop-hdds/test-utils/src/main/java/org/apache/ozone/test/GenericTestUtils.java#L214) instead of `Thread.sleep()`?
Thank you @hemantk-12 for review. I have handled the comment.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] devmadhuu commented on a diff in pull request #5217: Hdds 6646 - Intermittent failure in TestOzoneFileSystem#testRenameToTrashEnabled
Posted by "devmadhuu (via GitHub)" <gi...@apache.org>.
devmadhuu commented on code in PR #5217:
URL: https://github.com/apache/ozone/pull/5217#discussion_r1306417715
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/fs/ozone/TestOzoneFileSystem.java:
##########
@@ -802,6 +802,7 @@ protected void deleteRootDir() throws IOException, InterruptedException {
return;
}
deleteRootRecursively(fileStatuses);
+ Thread.sleep(500);
Review Comment:
> Can we use [GenericTestUtils#waitFor](https://github.com/apache/ozone/blob/387c53781aae03f944906a0add58f51250d36687/hadoop-hdds/test-utils/src/main/java/org/apache/ozone/test/GenericTestUtils.java#L214) 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] sadanand48 commented on a diff in pull request #5217: Hdds 6646 - Intermittent failure in TestOzoneFileSystem#testRenameToTrashEnabled
Posted by "sadanand48 (via GitHub)" <gi...@apache.org>.
sadanand48 commented on code in PR #5217:
URL: https://github.com/apache/ozone/pull/5217#discussion_r1307144675
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/fs/ozone/TestOzoneFileSystem.java:
##########
@@ -1618,15 +1621,13 @@ public void testGetTrashRoots() throws IOException {
* since fs.rename(src,dst,options) is enabled.
*/
@Test
- @Flaky("HDDS-6646")
public void testRenameToTrashEnabled() throws Exception {
Review Comment:
This particular test never calls deleteRootDir(), not sure how fixing deletRootDir makes this test non-flaky, Anyway I feel this functionality is tested in testTrash method in the same class in much detail . This test was written before the trash feature was complete, I feel we can remove this test from the class.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] hemantk-12 commented on a diff in pull request #5217: Hdds 6646 - Intermittent failure in TestOzoneFileSystem#testRenameToTrashEnabled
Posted by "hemantk-12 (via GitHub)" <gi...@apache.org>.
hemantk-12 commented on code in PR #5217:
URL: https://github.com/apache/ozone/pull/5217#discussion_r1308029483
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/fs/ozone/TestOzoneFileSystem.java:
##########
@@ -795,13 +796,15 @@ public void testListStatusOnKeyNameContainDelimiter() throws Exception {
*
* @throws IOException DB failure
*/
- protected void deleteRootDir() throws IOException, InterruptedException {
+ protected void deleteRootDir()
+ throws IOException, InterruptedException, TimeoutException {
FileStatus[] fileStatuses = fs.listStatus(ROOT);
if (fileStatuses == null) {
return;
}
deleteRootRecursively(fileStatuses);
+ Thread.sleep(500);
Review Comment:
+1
I don't get either why `TimeoutException` is added [here](https://github.com/apache/ozone/pull/5217/files#diff-0a2a989278da5b28bd0346a9db79c98ca319ff238eb150af17e2686afabfc1e5R800). May be DEv forgot to remove it while reverting some change.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] sadanand48 commented on a diff in pull request #5217: Hdds 6646 - Intermittent failure in TestOzoneFileSystem#testRenameToTrashEnabled
Posted by "sadanand48 (via GitHub)" <gi...@apache.org>.
sadanand48 commented on code in PR #5217:
URL: https://github.com/apache/ozone/pull/5217#discussion_r1309057054
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/fs/ozone/TestOzoneFileSystem.java:
##########
@@ -1618,15 +1621,13 @@ public void testGetTrashRoots() throws IOException {
* since fs.rename(src,dst,options) is enabled.
*/
@Test
- @Flaky("HDDS-6646")
public void testRenameToTrashEnabled() throws Exception {
Review Comment:
Thanks Devesh, sounds good to me.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] devmadhuu commented on pull request #5217: Hdds 6646 - Intermittent failure in TestOzoneFileSystem#testRenameToTrashEnabled
Posted by "devmadhuu (via GitHub)" <gi...@apache.org>.
devmadhuu commented on PR #5217:
URL: https://github.com/apache/ozone/pull/5217#issuecomment-1697872273
> Thanks for addressing review comments.
>
> Overall changes are fine.
>
> Few doubts and suggestions:
>
> 1. PR title is incorrect I think. It is supposed to be `HDDS-6646 Intermittent failure ....`
> 2. Can you please share the workflow run where you run the test 10 times? I found [this](https://github.com/devmadhuu/ozone/actions/runs/5938962749/job/16109360981), but in last iteration test still failed. Also I would suggest to run only `testRenameToTrashEnabled` and have a green CI for it.
> 3. I think [deleteRootDir](https://github.com/apache/ozone/blob/011de37b19ec874819ae5bbf726580ba5b0b7f93/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/fs/ozone/TestOzoneFileSystem.java#L721) is not needed here because it is deleted in [@After](https://github.com/apache/ozone/blob/011de37b19ec874819ae5bbf726580ba5b0b7f93/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/fs/ozone/TestOzoneFileSystem.java#L228). 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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] smengcl commented on a diff in pull request #5217: HDDS-6646. Intermittent failure in TestOzoneFileSystem#testRenameToTrashEnabled
Posted by "smengcl (via GitHub)" <gi...@apache.org>.
smengcl commented on code in PR #5217:
URL: https://github.com/apache/ozone/pull/5217#discussion_r1317650774
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/fs/ozone/TestOzoneFileSystem.java:
##########
@@ -802,20 +802,12 @@ protected void deleteRootDir() throws IOException, InterruptedException {
return;
}
deleteRootRecursively(fileStatuses);
-
- // 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();
Review Comment:
I'm not sure the removal of this line belongs to this PR?
#5244 still looks required.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] devmadhuu commented on a diff in pull request #5217: Hdds 6646 - Intermittent failure in TestOzoneFileSystem#testRenameToTrashEnabled
Posted by "devmadhuu (via GitHub)" <gi...@apache.org>.
devmadhuu commented on code in PR #5217:
URL: https://github.com/apache/ozone/pull/5217#discussion_r1309045844
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/fs/ozone/TestOzoneFileSystem.java:
##########
@@ -795,15 +796,19 @@ public void testListStatusOnKeyNameContainDelimiter() throws Exception {
*
* @throws IOException DB failure
*/
- protected void deleteRootDir() throws IOException, InterruptedException {
+ protected void deleteRootDir()
+ throws IOException, InterruptedException, TimeoutException {
FileStatus[] fileStatuses = fs.listStatus(ROOT);
if (fileStatuses == null) {
return;
}
deleteRootRecursively(fileStatuses);
- Thread.sleep(500);
fileStatuses = fs.listStatus(ROOT);
+ FileStatus[] finalFileStatuses = fileStatuses;
+ GenericTestUtils.waitFor(
Review Comment:
Thanks @hemantk-12 , Its handled as per your suggestion. Kindly re-review.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] devmadhuu commented on a diff in pull request #5217: Hdds 6646 - Intermittent failure in TestOzoneFileSystem#testRenameToTrashEnabled
Posted by "devmadhuu (via GitHub)" <gi...@apache.org>.
devmadhuu commented on code in PR #5217:
URL: https://github.com/apache/ozone/pull/5217#discussion_r1309158311
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/fs/ozone/TestOzoneFileSystem.java:
##########
@@ -1657,15 +1663,21 @@ public void testTrash() throws Exception {
isAssignableFrom(TrashPolicyOzone.class));
assertEquals(TRASH_INTERVAL, trash.getConf().
getFloat(OMConfigKeys.OZONE_FS_TRASH_INTERVAL_KEY, 0), 0);
- // Call moveToTrash. We can't call protected fs.rename() directly
- trash.moveToTrash(path);
// Construct paths
String username = UserGroupInformation.getCurrentUser().getShortUserName();
Path userTrash = new Path(TRASH_ROOT, username);
Path userTrashCurrent = new Path(userTrash, "Current");
Path trashPath = new Path(userTrashCurrent, testKeyName);
+ // 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
Review Comment:
> 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 testTrashw
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] devmadhuu commented on a diff in pull request #5217: Hdds 6646 - Intermittent failure in TestOzoneFileSystem#testRenameToTrashEnabled
Posted by "devmadhuu (via GitHub)" <gi...@apache.org>.
devmadhuu commented on code in PR #5217:
URL: https://github.com/apache/ozone/pull/5217#discussion_r1306364360
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/fs/ozone/TestOzoneFileSystem.java:
##########
@@ -802,6 +802,7 @@ protected void deleteRootDir() throws IOException, InterruptedException {
return;
}
deleteRootRecursively(fileStatuses);
+ Thread.sleep(500);
Review Comment:
> Can we use [GenericTestUtils#waitFor](https://github.com/apache/ozone/blob/387c53781aae03f944906a0add58f51250d36687/hadoop-hdds/test-utils/src/main/java/org/apache/ozone/test/GenericTestUtils.java#L214) instead of `Thread.sleep()`?
Thank you @hemantk-12 for review. I have handled the comment.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] devmadhuu commented on pull request #5217: HDDS-6646. Intermittent failure in TestOzoneFileSystem#testRenameToTrashEnabled
Posted by "devmadhuu (via GitHub)" <gi...@apache.org>.
devmadhuu commented on PR #5217:
URL: https://github.com/apache/ozone/pull/5217#issuecomment-1698447668
> > > Thanks for addressing review comments.
> > > Overall changes are fine.
> > > Few doubts and suggestions:
> > >
> > > 1. PR title is incorrect I think. It is supposed to be `HDDS-6646 Intermittent failure ....`
> > > 2. Can you please share the workflow run where you run the test 10 times? I found [this](https://github.com/devmadhuu/ozone/actions/runs/5938962749/job/16109360981), but in last iteration test still failed. Also I would suggest to run only `testRenameToTrashEnabled` and have a green CI for it.
> > > 3. I think [deleteRootDir](https://github.com/apache/ozone/blob/011de37b19ec874819ae5bbf726580ba5b0b7f93/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/fs/ozone/TestOzoneFileSystem.java#L721) is not needed here because it is deleted in [@After](https://github.com/apache/ozone/blob/011de37b19ec874819ae5bbf726580ba5b0b7f93/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/fs/ozone/TestOzoneFileSystem.java#L228). 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
>
> 1. 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
> 2. I am curious how `deleteRootDir()` calling solves the issue in test [testListStatusOnLargeDirectory](https://github.com/apache/ozone/blob/011de37b19ec874819ae5bbf726580ba5b0b7f93/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/fs/ozone/TestOzoneFileSystem.java#L720). Did you try to remove it and run? Do you have a run where it failed after it was removed?
1. I fixed the PR title. Thanks for pointing out.
2. `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.
3.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] sumitagrawl merged pull request #5217: HDDS-6646. Intermittent failure in TestOzoneFileSystem#testRenameToTrashEnabled
Posted by "sumitagrawl (via GitHub)" <gi...@apache.org>.
sumitagrawl merged PR #5217:
URL: https://github.com/apache/ozone/pull/5217
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] devmadhuu commented on pull request #5217: Hdds 6646 - Intermittent failure in TestOzoneFileSystem#testRenameToTrashEnabled
Posted by "devmadhuu (via GitHub)" <gi...@apache.org>.
devmadhuu commented on PR #5217:
URL: https://github.com/apache/ozone/pull/5217#issuecomment-1694313229
> 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](https://github.com/apache/ozone/pull/5204) 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] sadanand48 commented on pull request #5217: Hdds 6646 - Intermittent failure in TestOzoneFileSystem#testRenameToTrashEnabled
Posted by "sadanand48 (via GitHub)" <gi...@apache.org>.
sadanand48 commented on PR #5217:
URL: https://github.com/apache/ozone/pull/5217#issuecomment-1697810414
Thanks @devmadhuu , patch looks good to me, IIUC, there are 3 tests in question:
1. `testRenameToTrashEnabled` : which the current patch fixes.
2. `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
3. `testListStatusOnLargeDirectory` : which by fixing deleteRoot by adding a wait should stabilise the test further.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] hemantk-12 commented on a diff in pull request #5217: Hdds 6646 - Intermittent failure in TestOzoneFileSystem#testRenameToTrashEnabled
Posted by "hemantk-12 (via GitHub)" <gi...@apache.org>.
hemantk-12 commented on code in PR #5217:
URL: https://github.com/apache/ozone/pull/5217#discussion_r1308029483
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/fs/ozone/TestOzoneFileSystem.java:
##########
@@ -795,13 +796,15 @@ public void testListStatusOnKeyNameContainDelimiter() throws Exception {
*
* @throws IOException DB failure
*/
- protected void deleteRootDir() throws IOException, InterruptedException {
+ protected void deleteRootDir()
+ throws IOException, InterruptedException, TimeoutException {
FileStatus[] fileStatuses = fs.listStatus(ROOT);
if (fileStatuses == null) {
return;
}
deleteRootRecursively(fileStatuses);
+ Thread.sleep(500);
Review Comment:
+1
I don't get either why `TimeoutException` is added [here](https://github.com/apache/ozone/pull/5217/files#diff-0a2a989278da5b28bd0346a9db79c98ca319ff238eb150af17e2686afabfc1e5R800). May be you forgot to remove it while reverting it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] devmadhuu commented on pull request #5217: HDDS-6646. Intermittent failure in TestOzoneFileSystem#testRenameToTrashEnabled
Posted by "devmadhuu (via GitHub)" <gi...@apache.org>.
devmadhuu commented on PR #5217:
URL: https://github.com/apache/ozone/pull/5217#issuecomment-1699269935
Here is the successful green CI from fork [run](https://github.com/devmadhuu/ozone/actions/runs/6024556982) in fork.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] sadanand48 commented on a diff in pull request #5217: HDDS-6646. Intermittent failure in TestOzoneFileSystem#testRenameToTrashEnabled
Posted by "sadanand48 (via GitHub)" <gi...@apache.org>.
sadanand48 commented on code in PR #5217:
URL: https://github.com/apache/ozone/pull/5217#discussion_r1317107919
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/fs/ozone/TestOzoneFileSystem.java:
##########
@@ -1623,35 +1615,6 @@ public void testGetTrashRoots() throws IOException {
Assert.assertEquals(6, res.size());
}
- /**
- * Check that files are moved to trash.
- * since fs.rename(src,dst,options) is enabled.
- */
- @Test
- @Flaky("HDDS-6646")
- public void testRenameToTrashEnabled() throws Exception {
Review Comment:
Was the test removal intended? because earlier commit had fixed the test and description also says so.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] devmadhuu commented on a diff in pull request #5217: HDDS-6646. Intermittent failure in TestOzoneFileSystem#testRenameToTrashEnabled
Posted by "devmadhuu (via GitHub)" <gi...@apache.org>.
devmadhuu commented on code in PR #5217:
URL: https://github.com/apache/ozone/pull/5217#discussion_r1318053352
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/fs/ozone/TestOzoneFileSystem.java:
##########
@@ -802,20 +802,12 @@ protected void deleteRootDir() throws IOException, InterruptedException {
return;
}
deleteRootRecursively(fileStatuses);
-
- // 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();
Review Comment:
Yes @smengcl #5244 needs anyway. I think while resolving conflict, it got removed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] hemantk-12 commented on a diff in pull request #5217: Hdds 6646 - Intermittent failure in TestOzoneFileSystem#testRenameToTrashEnabled
Posted by "hemantk-12 (via GitHub)" <gi...@apache.org>.
hemantk-12 commented on code in PR #5217:
URL: https://github.com/apache/ozone/pull/5217#discussion_r1306015377
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/fs/ozone/TestOzoneFileSystem.java:
##########
@@ -802,6 +802,7 @@ protected void deleteRootDir() throws IOException, InterruptedException {
return;
}
deleteRootRecursively(fileStatuses);
+ Thread.sleep(500);
Review Comment:
Can we use [GenericTestUtils#waitFor](https://github.com/apache/ozone/blob/387c53781aae03f944906a0add58f51250d36687/hadoop-hdds/test-utils/src/main/java/org/apache/ozone/test/GenericTestUtils.java#L214) instead of `Thread.sleep()`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] hemantk-12 commented on a diff in pull request #5217: Hdds 6646 - Intermittent failure in TestOzoneFileSystem#testRenameToTrashEnabled
Posted by "hemantk-12 (via GitHub)" <gi...@apache.org>.
hemantk-12 commented on code in PR #5217:
URL: https://github.com/apache/ozone/pull/5217#discussion_r1308028257
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/fs/ozone/TestOzoneFileSystem.java:
##########
@@ -795,15 +796,19 @@ public void testListStatusOnKeyNameContainDelimiter() throws Exception {
*
* @throws IOException DB failure
*/
- protected void deleteRootDir() throws IOException, InterruptedException {
+ protected void deleteRootDir()
+ throws IOException, InterruptedException, TimeoutException {
FileStatus[] fileStatuses = fs.listStatus(ROOT);
if (fileStatuses == null) {
return;
}
deleteRootRecursively(fileStatuses);
- Thread.sleep(500);
fileStatuses = fs.listStatus(ROOT);
+ FileStatus[] finalFileStatuses = fileStatuses;
+ GenericTestUtils.waitFor(
Review Comment:
You are supposed to fetch `fs.listStatus(ROOT);` inside the lambda.
https://github.com/apache/ozone/pull/5217#discussion_r1308026768
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] devmadhuu commented on a diff in pull request #5217: Hdds 6646 - Intermittent failure in TestOzoneFileSystem#testRenameToTrashEnabled
Posted by "devmadhuu (via GitHub)" <gi...@apache.org>.
devmadhuu commented on code in PR #5217:
URL: https://github.com/apache/ozone/pull/5217#discussion_r1309041979
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/fs/ozone/TestOzoneFileSystem.java:
##########
@@ -1618,15 +1621,13 @@ public void testGetTrashRoots() throws IOException {
* since fs.rename(src,dst,options) is enabled.
*/
@Test
- @Flaky("HDDS-6646")
public void testRenameToTrashEnabled() throws Exception {
Review Comment:
Thanks Sadanand for review. As discussed, we need to test Trash related tests in separate Test class spinning separate mini cluster having emptier thread started with close match of timings of relevant assertion statements and corresponding trash and checkpoint interval. So I have moved assertion for above test in testTrash test case and will be handled as part of [HDDS-6645](https://issues.apache.org/jira/browse/HDDS-6645)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] hemantk-12 commented on a diff in pull request #5217: Hdds 6646 - Intermittent failure in TestOzoneFileSystem#testRenameToTrashEnabled
Posted by "hemantk-12 (via GitHub)" <gi...@apache.org>.
hemantk-12 commented on code in PR #5217:
URL: https://github.com/apache/ozone/pull/5217#discussion_r1309119690
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/fs/ozone/TestOzoneFileSystem.java:
##########
@@ -1627,19 +1633,19 @@ public void testRenameToTrashEnabled() throws Exception {
stream.write(1);
}
- // Call moveToTrash. We can't call protected fs.rename() directly
- trash.moveToTrash(path);
-
// Construct paths
String username = UserGroupInformation.getCurrentUser().getShortUserName();
Path userTrash = new Path(TRASH_ROOT, username);
- Path userTrashCurrent = new Path(userTrash, "Current");
- Path trashPath = new Path(userTrashCurrent, testKeyName);
- // Trash Current directory should still have been created.
- Assert.assertTrue(o3fs.exists(userTrashCurrent));
- // Check under trash, the key should be present
- Assert.assertTrue(o3fs.exists(trashPath));
+ Assert.assertFalse(o3fs.exists(userTrash));
+ // Call moveToTrash. We can't call protected fs.rename() directly
+ trash.moveToTrash(path);
+ // 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
Review Comment:
```suggestion
// possible flakiness is hard to avoid unless we test this test case
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] hemantk-12 commented on a diff in pull request #5217: Hdds 6646 - Intermittent failure in TestOzoneFileSystem#testRenameToTrashEnabled
Posted by "hemantk-12 (via GitHub)" <gi...@apache.org>.
hemantk-12 commented on code in PR #5217:
URL: https://github.com/apache/ozone/pull/5217#discussion_r1308026768
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/fs/ozone/TestOzoneFileSystem.java:
##########
@@ -802,6 +802,7 @@ protected void deleteRootDir() throws IOException, InterruptedException {
return;
}
deleteRootRecursively(fileStatuses);
+ Thread.sleep(500);
Review Comment:
I'm surprise that `Thread.sleep()` works fine but [GenericTestUtils#waitFor](https://github.com/apache/ozone/blob/387c53781aae03f944906a0add58f51250d36687/hadoop-hdds/test-utils/src/main/java/org/apache/ozone/test/GenericTestUtils.java#L214) 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);
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] sadanand48 commented on a diff in pull request #5217: Hdds 6646 - Intermittent failure in TestOzoneFileSystem#testRenameToTrashEnabled
Posted by "sadanand48 (via GitHub)" <gi...@apache.org>.
sadanand48 commented on code in PR #5217:
URL: https://github.com/apache/ozone/pull/5217#discussion_r1307152460
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/fs/ozone/TestOzoneFileSystem.java:
##########
@@ -1618,15 +1621,13 @@ public void testGetTrashRoots() throws IOException {
* since fs.rename(src,dst,options) is enabled.
*/
@Test
- @Flaky("HDDS-6646")
public void testRenameToTrashEnabled() throws Exception {
Review Comment:
I see that testTrash is also marked flaky in HDDS-6645. Can we please check if that test is not flaky now, If it isn't, we can keep it and remove this else we can enable this till that is enabled and then remove.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] hemantk-12 commented on a diff in pull request #5217: Hdds 6646 - Intermittent failure in TestOzoneFileSystem#testRenameToTrashEnabled
Posted by "hemantk-12 (via GitHub)" <gi...@apache.org>.
hemantk-12 commented on code in PR #5217:
URL: https://github.com/apache/ozone/pull/5217#discussion_r1309123556
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/fs/ozone/TestOzoneFileSystem.java:
##########
@@ -1657,15 +1663,21 @@ public void testTrash() throws Exception {
isAssignableFrom(TrashPolicyOzone.class));
assertEquals(TRASH_INTERVAL, trash.getConf().
getFloat(OMConfigKeys.OZONE_FS_TRASH_INTERVAL_KEY, 0), 0);
- // Call moveToTrash. We can't call protected fs.rename() directly
- trash.moveToTrash(path);
// Construct paths
String username = UserGroupInformation.getCurrentUser().getShortUserName();
Path userTrash = new Path(TRASH_ROOT, username);
Path userTrashCurrent = new Path(userTrash, "Current");
Path trashPath = new Path(userTrashCurrent, testKeyName);
+ // 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
Review Comment:
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?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] devmadhuu commented on a diff in pull request #5217: Hdds 6646 - Intermittent failure in TestOzoneFileSystem#testRenameToTrashEnabled
Posted by "devmadhuu (via GitHub)" <gi...@apache.org>.
devmadhuu commented on code in PR #5217:
URL: https://github.com/apache/ozone/pull/5217#discussion_r1309158311
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/fs/ozone/TestOzoneFileSystem.java:
##########
@@ -1657,15 +1663,21 @@ public void testTrash() throws Exception {
isAssignableFrom(TrashPolicyOzone.class));
assertEquals(TRASH_INTERVAL, trash.getConf().
getFloat(OMConfigKeys.OZONE_FS_TRASH_INTERVAL_KEY, 0), 0);
- // Call moveToTrash. We can't call protected fs.rename() directly
- trash.moveToTrash(path);
// Construct paths
String username = UserGroupInformation.getCurrentUser().getShortUserName();
Path userTrash = new Path(TRASH_ROOT, username);
Path userTrashCurrent = new Path(userTrash, "Current");
Path trashPath = new Path(userTrashCurrent, testKeyName);
+ // 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
Review Comment:
> 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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] hemantk-12 commented on a diff in pull request #5217: Hdds 6646 - Intermittent failure in TestOzoneFileSystem#testRenameToTrashEnabled
Posted by "hemantk-12 (via GitHub)" <gi...@apache.org>.
hemantk-12 commented on code in PR #5217:
URL: https://github.com/apache/ozone/pull/5217#discussion_r1308029483
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/fs/ozone/TestOzoneFileSystem.java:
##########
@@ -795,13 +796,15 @@ public void testListStatusOnKeyNameContainDelimiter() throws Exception {
*
* @throws IOException DB failure
*/
- protected void deleteRootDir() throws IOException, InterruptedException {
+ protected void deleteRootDir()
+ throws IOException, InterruptedException, TimeoutException {
FileStatus[] fileStatuses = fs.listStatus(ROOT);
if (fileStatuses == null) {
return;
}
deleteRootRecursively(fileStatuses);
+ Thread.sleep(500);
Review Comment:
+1
I don't get either why `TimeoutException` is added [here](https://github.com/apache/ozone/pull/5217/files#diff-0a2a989278da5b28bd0346a9db79c98ca319ff238eb150af17e2686afabfc1e5R800). May be @devmadhuu forgot to remove it while reverting some change.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] smengcl commented on a diff in pull request #5217: HDDS-6646. Intermittent failure in TestOzoneFileSystem#testRenameToTrashEnabled
Posted by "smengcl (via GitHub)" <gi...@apache.org>.
smengcl commented on code in PR #5217:
URL: https://github.com/apache/ozone/pull/5217#discussion_r1319201232
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/fs/ozone/TestOzoneFileSystem.java:
##########
@@ -802,20 +802,12 @@ protected void deleteRootDir() throws IOException, InterruptedException {
return;
}
deleteRootRecursively(fileStatuses);
-
- // 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();
Review Comment:
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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] devmadhuu commented on a diff in pull request #5217: HDDS-6646. Intermittent failure in TestOzoneFileSystem#testRenameToTrashEnabled
Posted by "devmadhuu (via GitHub)" <gi...@apache.org>.
devmadhuu commented on code in PR #5217:
URL: https://github.com/apache/ozone/pull/5217#discussion_r1317122259
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/fs/ozone/TestOzoneFileSystem.java:
##########
@@ -1623,35 +1615,6 @@ public void testGetTrashRoots() throws IOException {
Assert.assertEquals(6, res.size());
}
- /**
- * Check that files are moved to trash.
- * since fs.rename(src,dst,options) is enabled.
- */
- @Test
- @Flaky("HDDS-6646")
- public void testRenameToTrashEnabled() throws Exception {
Review Comment:
> Was the test removal intended? because earlier commit had fixed the test and description also says so.
Yes @sadanand48 , as [HDDS-6645](https://issues.apache.org/jira/browse/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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] devmadhuu commented on a diff in pull request #5217: HDDS-6646. Intermittent failure in TestOzoneFileSystem#testRenameToTrashEnabled
Posted by "devmadhuu (via GitHub)" <gi...@apache.org>.
devmadhuu commented on code in PR #5217:
URL: https://github.com/apache/ozone/pull/5217#discussion_r1318056748
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/fs/ozone/TestOzoneFileSystem.java:
##########
@@ -1623,35 +1615,6 @@ public void testGetTrashRoots() throws IOException {
Assert.assertEquals(6, res.size());
}
- /**
- * Check that files are moved to trash.
- * since fs.rename(src,dst,options) is enabled.
- */
- @Test
- @Flaky("HDDS-6646")
- public void testRenameToTrashEnabled() throws Exception {
Review Comment:
@smengcl @hemantk-12 , Thanks for noticing that. Updated the PR description.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] devmadhuu commented on pull request #5217: Hdds 6646 - Intermittent failure in TestOzoneFileSystem#testRenameToTrashEnabled
Posted by "devmadhuu (via GitHub)" <gi...@apache.org>.
devmadhuu commented on PR #5217:
URL: https://github.com/apache/ozone/pull/5217#issuecomment-1697728984
@sumitagrawl @hemantk-12 @sadanand48 Kindly re-review.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] hemantk-12 commented on a diff in pull request #5217: Hdds 6646 - Intermittent failure in TestOzoneFileSystem#testRenameToTrashEnabled
Posted by "hemantk-12 (via GitHub)" <gi...@apache.org>.
hemantk-12 commented on code in PR #5217:
URL: https://github.com/apache/ozone/pull/5217#discussion_r1308026768
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/fs/ozone/TestOzoneFileSystem.java:
##########
@@ -802,6 +802,7 @@ protected void deleteRootDir() throws IOException, InterruptedException {
return;
}
deleteRootRecursively(fileStatuses);
+ Thread.sleep(500);
Review Comment:
I'm surprise that `Thread.sleep()` works fine but [GenericTestUtils#waitFor](https://github.com/apache/ozone/blob/387c53781aae03f944906a0add58f51250d36687/hadoop-hdds/test-utils/src/main/java/org/apache/ozone/test/GenericTestUtils.java#L214) 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) {
return false;
}
}, 100, 500);
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] hemantk-12 commented on pull request #5217: Hdds 6646 - Intermittent failure in TestOzoneFileSystem#testRenameToTrashEnabled
Posted by "hemantk-12 (via GitHub)" <gi...@apache.org>.
hemantk-12 commented on PR #5217:
URL: https://github.com/apache/ozone/pull/5217#issuecomment-1696524953
> > 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](https://github.com/apache/ozone/pull/5204) 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?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] hemantk-12 commented on a diff in pull request #5217: Hdds 6646 - Intermittent failure in TestOzoneFileSystem#testRenameToTrashEnabled
Posted by "hemantk-12 (via GitHub)" <gi...@apache.org>.
hemantk-12 commented on code in PR #5217:
URL: https://github.com/apache/ozone/pull/5217#discussion_r1308029483
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/fs/ozone/TestOzoneFileSystem.java:
##########
@@ -795,13 +796,15 @@ public void testListStatusOnKeyNameContainDelimiter() throws Exception {
*
* @throws IOException DB failure
*/
- protected void deleteRootDir() throws IOException, InterruptedException {
+ protected void deleteRootDir()
+ throws IOException, InterruptedException, TimeoutException {
FileStatus[] fileStatuses = fs.listStatus(ROOT);
if (fileStatuses == null) {
return;
}
deleteRootRecursively(fileStatuses);
+ Thread.sleep(500);
Review Comment:
+1
I don't get either why `TimeoutException` is added [here](https://github.com/apache/ozone/pull/5217/files#diff-0a2a989278da5b28bd0346a9db79c98ca319ff238eb150af17e2686afabfc1e5R800). May be DEv forgot to remove it while reverting it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org