[FLINK-20428][test] Fix the unstable test ZooKeeperLeaderElectionConnectionHandlingTest#testConnectionSuspendedHandlingDuringInitialization #14268
Conversation
|
Thanks a lot for your contribution to the Apache Flink project. I'm the @flinkbot. I help the community Automated ChecksLast check on commit 00f90b8 (Tue Dec 01 06:10:09 UTC 2020) Warnings:
Mention the bot in a comment to re-run the automated checks. Review Progress
Please see the Pull Request Review Guide for a full explanation of the review process. The Bot is tracking the review progress through labels. Labels are applied according to the order of the review items. For consensus, approval by a Flink committer of PMC member is required Bot commandsThe @flinkbot bot supports the following commands:
|
|
cc @XComp @tillrohrmann Could you please have a look? |
There was a problem hiding this comment.
Thanks for opening this PR @wangyang0918. I think we can further simplify the QueueLeaderElectionListener to not have a timeout field at all. Please take a look at my comments.
| @@ -281,6 +282,10 @@ public void notifyLeaderAddress(LeaderInformation leaderInformation) { | |||
| } | |||
|
|
|||
| public CompletableFuture<String> next() { | |||
| return next(timeout); | |||
There was a problem hiding this comment.
Can we clean up the timeout field of the QueueLeaderElectionListener? I think it does not make sense that the leader election listener has such a field.
There was a problem hiding this comment.
Also, notifyLeaderAddress should probably wait until it can add the leaderInformation to the queue.
| return next(timeout); | ||
| } | ||
|
|
||
| public CompletableFuture<String> next(Duration timeout) { |
There was a problem hiding this comment.
| public CompletableFuture<String> next(Duration timeout) { | |
| public CompletableFuture<String> next(@Nullable Duration timeout) { |
| assertThat("No result is expected since there was no leader elected before stopping the server, yet.", | ||
| secondAddress, is(nullValue())); | ||
| // QueueLeaderElectionListener will be notified with an empty leader when ZK connection is suspended | ||
| final CompletableFuture<String> secondAddress = queueLeaderElectionListener.next(Duration.ofSeconds(3)); |
There was a problem hiding this comment.
If we are expecting an empty leader information, then a timeout is not required here.
wangyang0918
force-pushed
the
FLINK-20428-fix-zk-test
branch
from
00f90b8
to
8fc1003
Compare
…ectionHandlingTest#testConnectionSuspendedHandlingDuringInitialization
wangyang0918
force-pushed
the
FLINK-20428-fix-zk-test
branch
from
8fc1003
to
6afa53e
Compare
|
@tillrohrmann Thanks for the review. I have addressed the comments. |
There was a problem hiding this comment.
Thanks for updating this PR @wangyang0918. LGTM. I will merge this PR once AZP gives green light.
|
I went through the code and have nothing to add. Thanks @wangyang0918 |
…ectionHandlingTest#testConnectionSuspendedHandlingDuringInitialization This closes #14268.
When the ZooKeeper connection is suspended, the
LeaderRetrievalEventHandlerwill be notified with an empty leader information. In the failed testZooKeeperLeaderElectionConnectionHandlingTest.testConnectionSuspendedHandlingDuringInitialization, we expect no leader will be notified. This is wrong. The reason why it could work is the timeout is set to very small(50ms) and it is too fast to get the leader information.This PR will fix the unstable test via correcting the expectation.