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-31077][runtime] Mark pending checkpoint onCompletionPromise complete only after the completed checkpoint is added to the store. #21943
Conversation
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.
Thank @JunRuiLee for fixing this problem so quickly. I left some comments, please take a look~
flink-runtime/src/test/java/org/apache/flink/runtime/scheduler/DefaultSchedulerTest.java
Outdated
Show resolved
Hide resolved
flink-runtime/src/test/java/org/apache/flink/runtime/scheduler/DefaultSchedulerTest.java
Show resolved
Hide resolved
flink-runtime/src/test/java/org/apache/flink/runtime/scheduler/DefaultSchedulerTest.java
Outdated
Show resolved
Hide resolved
flink-runtime/src/test/java/org/apache/flink/runtime/scheduler/DefaultSchedulerTest.java
Outdated
Show resolved
Hide resolved
flink-runtime/src/test/java/org/apache/flink/runtime/scheduler/DefaultSchedulerTest.java
Outdated
Show resolved
Hide resolved
flink-runtime/src/main/java/org/apache/flink/runtime/checkpoint/PendingCheckpoint.java
Outdated
Show resolved
Hide resolved
flink-runtime/src/test/java/org/apache/flink/runtime/checkpoint/PendingCheckpointTest.java
Outdated
Show resolved
Hide resolved
flink-runtime/src/test/java/org/apache/flink/runtime/scheduler/DefaultSchedulerTest.java
Show resolved
Hide resolved
flink-runtime/src/test/java/org/apache/flink/runtime/scheduler/DefaultSchedulerTest.java
Outdated
Show resolved
Hide resolved
flink-runtime/src/main/java/org/apache/flink/runtime/checkpoint/CheckpointCoordinator.java
Outdated
Show resolved
Hide resolved
6d9a414
to
613bbd9
Compare
@reswqa Thanks for review and I've addressed all comments. PTAL! |
613bbd9
to
57dc6da
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.
Thanks @JunRuiLee for addressing all comments, I only left minor comments, PTAL~
...a/org/apache/flink/runtime/scheduler/exceptionhistory/ExceptionHistoryEntryTestingUtils.java
Outdated
Show resolved
Hide resolved
flink-runtime/src/main/java/org/apache/flink/runtime/checkpoint/PendingCheckpoint.java
Outdated
Show resolved
Hide resolved
fcb7a60
to
98dca84
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.
Thanks @JunRuiLee for addressing all my comments, LGTM. For safety, we may need @zhuzhurk to confirm the changes about CheckpointCoordinator
and PendingCheckpoint
.
@gaoyunhaii would you take a look at this change of checkpointing? |
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 for creating this PR!
I have a few comments. Please take a look.
flink-runtime/src/main/java/org/apache/flink/runtime/checkpoint/CheckpointCoordinator.java
Outdated
Show resolved
Hide resolved
flink-runtime/src/main/java/org/apache/flink/runtime/checkpoint/PendingCheckpoint.java
Show resolved
Hide resolved
flink-runtime/src/test/java/org/apache/flink/runtime/scheduler/DefaultSchedulerTest.java
Outdated
Show resolved
Hide resolved
@zhuzhurk Thanks for CR and I've addressed all comments, PTAL! |
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.
flink-runtime/src/main/java/org/apache/flink/runtime/checkpoint/CheckpointCoordinator.java
Show resolved
Hide resolved
@gaoyunhaii Thanks for CR and I've addressed comment, PTAL! |
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 @JunRuiLee for the fix, LGTM
…mplete only after the completed checkpoint is added to the store.
de1c890
to
ca6c9d4
Compare
@flinkbot run azure |
…mplete only after the completed checkpoint is added to the checkpoint store. This closes #21943.
…mplete only after the completed checkpoint is added to the checkpoint store. This closes #21943.
What is the purpose of the change
Mark pending checkpoint onCompletionPromise complete only after the completed checkpoint is added to the store.
Brief change log
Mark pending checkpoint onCompletionPromise complete only after the completed checkpoint is added to the store.
Verifying this change
This change added tests and can be verified as follows: DefaultSchedulerTest#testTriggerCheckpointAndCompletedAfterStore
Does this pull request potentially affect one of the following parts:
@Public(Evolving)
: (yes / no)Documentation