-
Notifications
You must be signed in to change notification settings - Fork 587
HDDS-4176. Fix failed UT: test2WayCommitForTimeoutException #1370
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
HDDS-4176. Fix failed UT: test2WayCommitForTimeoutException #1370
Conversation
|
@elek Could you help review this patch ? Thank you very much. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This logic looks right to me.
Can you bump up the number of DN to make tests run against higher number DN: https://github.com/apache/hadoop-ozone/blob/34ee8311b0d0a37878fe1fd2e5d8c1b91aa8cc8f/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/client/rpc/Test2WayCommitInRatis.java#L109
Which seems will cover your change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@amaliujia Thanks for your review. TestWatchForCommit already startCluster with 9 nodes, maybe it is enough.
https://github.com/apache/hadoop-ozone/blob/master/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/client/rpc/TestWatchForCommit.java#L124
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops. You are right. I was looking at a wrong file.
adoroszlai
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @runzhiwang for fixing this long standing intermittent failure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: nodesInPipeline may be a better name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@adoroszlai Thanks for review. I have updated the patch.
e9d0ddc to
391ef3d
Compare
|
Thanks @runzhiwang for the fix and @amaliujia for the review. |
What changes were proposed in this pull request?
What's the problem ?

What's the reason of the failed ut ?
When cluster create 9 nodes, the following code try to shutdown the follower of the pipeline. Actually, the pipeline only exist in 3 nodes, but the following code check 9 nodes, so the pipeline can not found in the other 6 nodes.
What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-4176
How was this patch tested?
Existed ut.