- Notifications
You must be signed in to change notification settings - Fork13.8k
[FLINK-38403][tests] Fix the unexpected test that the second job does not restore from checkpoint#27254
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:master
Are you sure you want to change the base?
Conversation
flinkbot commentedNov 19, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
| conf.set(StateRecoveryOptions.SAVEPOINT_PATH,restoreCheckpoint.toURI().toString()); | ||
| conf.set(StateRecoveryOptions.SAVEPOINT_PATH,restoreCheckpoint); |
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.
It adds the wrong prefix, so updatedrestoreCheckpoint to String.
AHeise left a comment
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 you very much for fixing this test! Left two remarks that need to be addressed before approval.
| assertNotNull( | ||
| "First job must generate a checkpoint for rescale test to be valid.", | ||
| checkpointDir); |
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.
Please use assertj (assertThat(checkpointDir).as("First job must generate a checkpoint for rescale test to be valid.").isNotNull)
| returnCommonTestUtils.getLatestCompletedCheckpointPath( | ||
| jobID,miniCluster.getMiniCluster()) | ||
| .map(File::new) | ||
| .orElseThrow(() ->newAssertionError("Could not generate checkpoint")); |
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 weFail.fail("Expected exception") here?
80692f1 to4e675d2Compare
1996fanrui left a comment
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@AHeise for the quick review, all comments are addressed.
4e675d2 to736ad29Comparesnuyanzin commentedNov 20, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
fyi: to have green ci, rebase to the latest master |
1996fanrui commentedNov 21, 2025
Thanks@snuyanzin for the reminder, I am still changing the PR, and I will rebase master branch before next push. |
… not restore from checkpoint
…ished eventuallyAdd comments to help understand the UnalignedCheckpointRescaleITCase
736ad29 to8938499Compare| * <p>Postscale phase: Job restores from checkpoint with different parallelism, failovers once, | ||
| * and finishes after source generates all records. |
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.
The second job expects failover once, and finishes after source generates all records. So removing@ThrowableAnnotation(ThrowableType.NonRecoverableError) forTestException.
Also, I introducedExpectedFinalJobStatus inUnalignedSettings to check the finalJobStatus.
What is the purpose of the change
Get more fromhttps://github.com/apache/flink/pull/27119/files#r2542189639
Brief change log
[FLINK-38403][tests] Fix the unexpected test that the second job does not restore from checkpoint