HDDS-8374. Fix Flaky Unit Test for TestContainerStateCounts in Recon #4534

ArafatKhan2198 · 2023-04-04T14:35:44Z

What changes were proposed in this pull request?

The testDeletedContainerCount() method of TestContainerStateCounts is considered flaky due to the processing order of the container report. If the pipeline does not exist, then the container is not added until the next container report is processed.

What is the link to the Apache JIRA

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

How was this patch tested?

Ran the UT several times to make sure it's not flaky

adoroszlai

Thanks @ArafatKhan2198 for the fix. I'll try repeated runs. In the meantime, please fix checkstyle error.

adoroszlai · 2023-04-04T14:58:20Z

...op-ozone/recon/src/test/java/org/apache/hadoop/ozone/recon/api/TestContainerStateCounts.java

+    // then container is not added until the next container report is processed
+    StorageContainerDatanodeProtocolProtos.SCMHeartbeatRequestProto
+        heartbeatRequestProto =
+        StorageContainerDatanodeProtocolProtos.SCMHeartbeatRequestProto.newBuilder()


hadoop-ozone/recon/src/test/java/org/apache/hadoop/ozone/recon/api/TestContainerStateCounts.java 406: Line is longer than 80 characters (found 84).

Thanks for pointing it out. For some reason the ./checkStyle script in dev tools did not find it out. But I have changed it

adoroszlai · 2023-04-04T15:13:22Z

@ArafatKhan2198 seems like the test failed in the first run:

Error:  Tests run: 1, Failures: 0, Errors: 1, Skipped: 0, Time elapsed: 37.517 s <<< FAILURE! - in org.apache.hadoop.ozone.recon.api.TestContainerStateCounts
Error:  org.apache.hadoop.ozone.recon.api.TestContainerStateCounts.testContainerStatsCount  Time elapsed: 37.468 s  <<< ERROR!
java.util.concurrent.TimeoutException: timeout: after 30000 millis
	at org.apache.ozone.test.LambdaTestUtils$GenerateTimeout.evaluate(LambdaTestUtils.java:677)
	at org.apache.ozone.test.LambdaTestUtils.await(LambdaTestUtils.java:154)
	at org.apache.ozone.test.LambdaTestUtils.await(LambdaTestUtils.java:192)
	at org.apache.hadoop.ozone.recon.api.TestContainerStateCounts.waitAndCheckConditionAfterHeartbeat(TestContainerStateCounts.java:413)
	at org.apache.hadoop.ozone.recon.api.TestContainerStateCounts.testContainerStatsCount(TestContainerStateCounts.java:369)

https://github.com/ArafatKhan2198/ozone/actions/runs/4608724963/jobs/8144896789#step:6:2706

ArafatKhan2198 · 2023-04-04T16:32:40Z

@ArafatKhan2198 seems like the test failed in the first run:

Error:  Tests run: 1, Failures: 0, Errors: 1, Skipped: 0, Time elapsed: 37.517 s <<< FAILURE! - in org.apache.hadoop.ozone.recon.api.TestContainerStateCounts
Error:  org.apache.hadoop.ozone.recon.api.TestContainerStateCounts.testContainerStatsCount  Time elapsed: 37.468 s  <<< ERROR!
java.util.concurrent.TimeoutException: timeout: after 30000 millis
	at org.apache.ozone.test.LambdaTestUtils$GenerateTimeout.evaluate(LambdaTestUtils.java:677)
	at org.apache.ozone.test.LambdaTestUtils.await(LambdaTestUtils.java:154)
	at org.apache.ozone.test.LambdaTestUtils.await(LambdaTestUtils.java:192)
	at org.apache.hadoop.ozone.recon.api.TestContainerStateCounts.waitAndCheckConditionAfterHeartbeat(TestContainerStateCounts.java:413)
	at org.apache.hadoop.ozone.recon.api.TestContainerStateCounts.testContainerStatsCount(TestContainerStateCounts.java:369)

https://github.com/ArafatKhan2198/ozone/actions/runs/4608724963/jobs/8144896789#step:6:2706

My apologise @adoroszlai. By mistake, I reverted back to the previous value after running the tests successfully. Please run the tests now. I have already run them a total of 20 times, and they have passed.

adoroszlai · 2023-04-04T17:10:49Z

Thanks @ArafatKhan2198 for the update, but the test is still flaky, failed in 14/100 runs:

https://github.com/adoroszlai/hadoop-ozone/actions/runs/4610115793

adoroszlai · 2023-04-11T07:29:48Z

Thanks @ArafatKhan2198 for updating the patch. Have you run repeated tests in CI? Can you please share the link to the CI run?

ArafatKhan2198 · 2023-04-11T15:49:01Z

https://github.com/adoroszlai/hadoop-ozone/actions/runs/4610115793

Here you go @adoroszlai :- https://github.com/ArafatKhan2198/ozone/actions/runs/4669283289

adoroszlai · 2023-04-11T15:51:14Z

Thanks @ArafatKhan2198 for updating the patch and for repeated runs, the results look good. I'll defer review of the actual fix to folks more familiar with Recon.

ArafatKhan2198 · 2023-04-11T15:52:42Z

@devmadhuu @adoroszlai Can you please take a look

smengcl

Thanks @ArafatKhan2198 . lgtm

adoroszlai · 2023-04-12T08:09:41Z

Please try to avoid force-push when updating the PR. Here are some great articles that explain why:

https://developers.mattermost.com/blog/submitting-great-prs/#4-avoid-force-pushing
https://www.freecodecamp.org/news/optimize-pull-requests-for-reviewer-happiness#request-a-review

adoroszlai · 2023-04-12T14:01:06Z

@ArafatKhan2198 I may be missing something, but the latest commit 3fb721a doesn't seem to be related to the fix.

ArafatKhan2198 · 2023-04-12T14:10:49Z

@ArafatKhan2198 I may be missing something, but the latest commit 3fb721a doesn't seem to be related to the fix.

Hi @adoroszlai
In the pull request HDDS-7994, I have added a few more parameters to the JSON response of the clusterStateEndpoint of Recon. One of these parameters is keys pending for deletion. When we decided to expose this information to the UI, there was a suggestion to change the response parameter "deletedKeys" to "keysAwaitingDeletion". This parameter shows the keys in the deleted table waiting to be deleted by the keyDeletionService running in the background. Since HDDS-7994 has already been merged, I did not get a chance to change the response. Therefore, I have decided to incorporate this small name change in this PR.

adoroszlai · 2023-04-12T14:18:35Z

Therefore, I have decided to incorporate this small name change in this PR.

It would be better to avoid incorporating unrelated changes.

We already had successful CI for this PR, could have been merged. Now we need to trigger another run.
Commit message would be confusing (fix flaky test vs. rename JSON element).
CI needs to run fewer checks if only unit test is changed.

ArafatKhan2198 · 2023-04-12T14:31:53Z

Therefore, I have decided to incorporate this small name change in this PR.

It would be better to avoid incorporating unrelated changes.

We already had successful CI for this PR, could have been merged. Now we need to trigger another run.

Commit message would be confusing (fix flaky test vs. rename JSON element).

CI needs to run fewer checks if only unit test is changed.

Understood, I have removed those changes.
Thanks for the feedback

adoroszlai added test recon labels Apr 4, 2023

adoroszlai reviewed Apr 4, 2023

View reviewed changes

adoroszlai marked this pull request as draft April 4, 2023 17:10

ArafatKhan2198 marked this pull request as ready for review April 11, 2023 07:10

ArafatKhan2198 requested a review from adoroszlai April 11, 2023 07:26

adoroszlai requested review from dombizita and smengcl April 11, 2023 15:50

adoroszlai removed their request for review April 11, 2023 15:51

smengcl approved these changes Apr 12, 2023

View reviewed changes

ArafatKhan2198 added 6 commits April 12, 2023 13:32

HDDS-8374. Fix Flaky Unit Test for TestContainerStateCounts in Recon

9ab4cb6

Fixed checkstyle issues

274c2f5

Fixed the failing UT

b510e68

Fixed tests

7c38630

Fixed bugs

6637240

Fixes checkstyle and bugs

4fb868f

ArafatKhan2198 force-pushed the HDDS-8374 branch from 9d251f6 to 4fb868f Compare April 12, 2023 08:03

Enabled tests

6cc90ff

Made changes to endpoint for HDDS-8399

3fb721a

Removed unnecessary code changes

ad43ecd

adoroszlai merged commit bbcb810 into apache:master Apr 12, 2023

HDDS-8374. Fix Flaky Unit Test for TestContainerStateCounts in Recon #4534

HDDS-8374. Fix Flaky Unit Test for TestContainerStateCounts in Recon #4534

Uh oh!

Conversation

ArafatKhan2198 commented Apr 4, 2023

What changes were proposed in this pull request?

What is the link to the Apache JIRA

How was this patch tested?

Uh oh!

adoroszlai left a comment

Choose a reason for hiding this comment

Uh oh!

adoroszlai Apr 4, 2023

Choose a reason for hiding this comment

Uh oh!

ArafatKhan2198 Apr 4, 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!

adoroszlai commented Apr 4, 2023

Uh oh!

ArafatKhan2198 commented Apr 4, 2023

Uh oh!

adoroszlai commented Apr 4, 2023

Uh oh!

adoroszlai commented Apr 11, 2023

Uh oh!

ArafatKhan2198 commented Apr 11, 2023

Uh oh!

adoroszlai commented Apr 11, 2023

Uh oh!

ArafatKhan2198 commented Apr 11, 2023

Uh oh!

smengcl left a comment

Choose a reason for hiding this comment

Uh oh!

adoroszlai commented Apr 12, 2023

Uh oh!

adoroszlai commented Apr 12, 2023

Uh oh!

ArafatKhan2198 commented Apr 12, 2023

Uh oh!

adoroszlai commented Apr 12, 2023

Uh oh!

ArafatKhan2198 commented Apr 12, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ArafatKhan2198 Apr 4, 2023 •

edited

Loading