1996fanrui commented on code in PR #24246:
URL: https://github.com/apache/flink/pull/24246#discussion_r1475645896
##########
flink-runtime/src/test/java/org/apache/flink/runtime/testutils/CommonTestUtils.java:
##########
@@ -367,9 +367,11 @@ public static void waitForCheckpoint(JobID jobID,
MiniCluster miniCluster, int n
});
}
- /** Wait for on more completed checkpoint. */
- public static void waitForOneMoreCheckpoint(JobID jobID, MiniCluster
miniCluster)
- throws Exception {
+ /**
+ * Wait for a new completed checkpoint. Note: we wait for 2 or more
checkpoint to ensure the
+ * latest checkpoint start after waitForTwoOrMoreCheckpoint is called.
+ */
+ public static void waitForNewCheckpoint(JobID jobID, MiniCluster
miniCluster) throws Exception {
Review Comment:
> That way, we can pass in 2 in the test implementation analogously to
waitForCheckpoint. I also feel like we can remove some redundant code within
the two methods. 🤔
IIUC, the semantic between `waitForCheckpoint` and
`waitForOneMoreCheckpoint` are different. (`waitForOneMoreCheckpoint` is
renamed to `waitForNewCheckpoint` in this PR.)
- `waitForCheckpoint` check the total count of all completed checkpoints.
- `waitForOneMoreCheckpoint` check the whether the new checkpoint is
completed after it's called.
- For example, the job has 10 completed checkpoint before it's called.
- `waitForOneMoreCheckpoint` will wait for checkpoint-11 is completed.
BTW, I have refactored the `waitForNewCheckpoint`. I check the checkpoint
trigger time instead of checkpointCount.
I think checking trigger time is clearer than checkpointCount >= 2. Other
developers might don't know why check 2 checkpoint here, and `checkpointCount
>= 2` doesn't work when enabling the concurrent checkpoint.
WDYT?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscr...@flink.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
