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
[FLINK-12313] Add workaround to avoid race condition in SynchronousCheckpointITCase test #8602
[FLINK-12313] Add workaround to avoid race condition in SynchronousCheckpointITCase test #8602
Conversation
Thanks a lot for your contribution to the Apache Flink project. I'm the @flinkbot. I help the community 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:
|
@flinkbot attention @pnowojski @StefanRRichter |
004bb82
to
5870222
Compare
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.
I've left couple of questions.
...java/src/test/java/org/apache/flink/streaming/runtime/tasks/SynchronousCheckpointITCase.java
Outdated
Show resolved
Hide resolved
...java/src/test/java/org/apache/flink/streaming/runtime/tasks/SynchronousCheckpointITCase.java
Show resolved
Hide resolved
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.
LGTM % timeout. If you don't want to change it, let me know and I will merge it, but in that case please monitor jira issues if someone is reporting this test as flaky.
...java/src/test/java/org/apache/flink/streaming/runtime/tasks/SynchronousCheckpointITCase.java
Outdated
Show resolved
Hide resolved
...java/src/test/java/org/apache/flink/streaming/runtime/tasks/SynchronousCheckpointITCase.java
Show resolved
Hide resolved
5870222
to
9d0ce4d
Compare
@pnowojski, thank you for the review and feedback. I've updated the test timeout to 10 seconds, and propose to stick with it for now. |
I'll merge it once travis is green (previous build has failed on some unrelated issue). |
Travis is green, merging. |
What is the purpose of the change
The
SynchronousCheckpointITCase
has a race condition and with some chance may fail on random tests on CI. This PR adds an additional synchronization point as a workaround to avoid the issue.Additionally, the PR simplifies (refactors) the test, although the race condition is present in both refactored and non-refactored versions.
Brief change log
advanceToEndOfEventTime
method, to make sure thattriggerCheckpoint
thread has progressed far enough, before triggeringnotifyCheckpointComplete
.Verifying this change
This change is a trivial rework / code cleanup without any test coverage.
Does this pull request potentially affect one of the following parts:
@Public(Evolving)
: (yes / no)Documentation