-
Notifications
You must be signed in to change notification settings - Fork 13k
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-35379][Checkpoint] Fix incorrect checkpoint notification handling in file merging #24806
Conversation
@flinkbot run azure |
…ing in file merging
@flinkbot run azure |
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 the PR. I left two comments.
File manager cannot recognize the
PlaceholderStreamStateHandle
Theoretically, PlaceholderStreamStateHandle
should not be written to the meta file, and File manager should not know it.
.map(handle -> (SegmentFileStateHandle) handle); | ||
System.out.println("Restoring from checkpoint " + checkpointId); |
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.
LOG.info()?
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.
Debug info attached by mistake. Removed.
notifiedCheckpoint.add(checkpointId); | ||
discardCheckpoint(checkpointId); | ||
if (notifiedCheckpoint.size() > NUM_GHOST_CHECKPOINT_IDS) { | ||
notifiedCheckpoint.pollFirst(); |
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.
Should the corresponding checkpoint also be removed from notifiedSubtaskCheckpoint
?
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.
Added in notifyReleaseCheckpoint
and tryDiscardCheckpoint
This statement only applies for the |
@fredia Thanks for your comments! I've addressed them and 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.
@Zakelly Thanks for the update, LGTM.
What is the purpose of the change
This PR fix several issues with file merging mechanism, see
Brief change log
.Brief change log
Fix following issues:
PlaceholderStreamStateHandle
EmptySegmentFileStateHandle
out during recoveryVerifying this change
UT modified.
Does this pull request potentially affect one of the following parts:
@Public(Evolving)
: noDocumentation