[FLINK-10482] Fix double counting of checkpoint stat #7118
Conversation
azagrebin
force-pushed
the
FLINK-10482
branch
from
f09a853 to
dc836d7
Compare
tillrohrmann
left a comment
tillrohrmann
left a comment
There was a problem hiding this comment.
Thanks for opening this PR @azagrebin. I had some minor comments and a question. Before merging this PR it would be great if you could address them.
| numFailedCheckpoints); | ||
| } | ||
|
|
||
| private boolean assertDecrementOfInProgressCheckpointsNumber() { |
There was a problem hiding this comment.
maybe rename method into canDecrementOfInProgressCheckpointsNumber.
| String errorMessage = "Incremented the completed number of checkpoints " + | ||
| "without incrementing the in progress checkpoints before."; | ||
| LOG.warn(errorMessage); | ||
| LOG.debug("Inconsistent CheckpointStatsCounts", new IllegalStateException(errorMessage)); |
There was a problem hiding this comment.
Let's remove this logging statement.
There was a problem hiding this comment.
thanks, debugging leftover
| }, getMainThreadExecutor()) | ||
| .exceptionally(throwable -> { | ||
| if (cancelJob) { | ||
| if (cancelJob && executionGraph.getState() == JobStatus.RUNNING) { |
There was a problem hiding this comment.
What situation are we filtering out with this change?
| new ArrayList<>(pendingCheckpoints.values()); | ||
| pendingCheckpoints.clear(); | ||
|
|
||
| for (PendingCheckpoint p : pendingCheckpointsSnapshot) { |
There was a problem hiding this comment.
Just as a note, I guess we could have solved the problem also by changing JobMaster.java:965-975 into a handleAsync part because the problem seems to be that by aborting a PendingCheckpoint we fail the savepoint future which is then directly triggers the execution of the exceptionally handler. By decoupling the exceptionally handler from the stopCheckpointScheduler call, there should not be any concurrency problem.
There was a problem hiding this comment.
Agree, this fix will solve the original problem without other changes.
|
Thank you for the review @tillrohrmann ! I addressed the comments. |
tillrohrmann
left a comment
tillrohrmann
left a comment
There was a problem hiding this comment.
Thanks for updating the PR @azagrebin. LGTM. Merging.
|
Manually merged. |
3 participants
What is the purpose of the change
This PR fixes double counting of checkpoints in progress in their statistics.
Brief change log
Verifying this change
submit DataStreamAllroundTestProgram in a loop in aws emr, wait for failure, check there was no negative number of checkpoints logs
Does this pull request potentially affect one of the following parts:
@Public(Evolving): (no)Documentation