SEZ9 commented on PR #12448: URL: https://github.com/apache/seatunnel/pull/12448#issuecomment-5903290767
Thanks for the rerun report at a47871252, @hutiefang76 (run 35994809579). Both engine-v2 IT shards staying green while the six remaining red groups sit outside the checkpoint module is consistent with keeping this patch scoped to the checkpoint fix. And thanks @yigitcan-ozturk for reviewing a47871252d6e2ea1215dfd6eab12a2ae7c3ad64e — agreed that the scheduled callback is the right boundary and that the FAILED + `handleCheckpointError(...)` assertions cover the new failure contract directly. My earlier approval still stands. For completeness, the three follow-up points from my previous review are listed below as non-blocking suggestions; if any are already addressed at the current head, please ignore them (I have not re-diffed the head against them): 1. **Test placement** — `CheckpointCoordinatorScheduledTriggerTest` re-declares a `buildMinimalCoordinator` fixture that already lives in `CheckpointCoordinatorTest`. Folding the scenario into the existing class would leave one fixture to maintain. 2. **Javadoc** — since `scheduleTriggerPendingCheckpoint` now carries error-handling semantics (any `Throwable` from the trigger moves the coordinator to FAILED and notifies the manager), a method-level Javadoc would match how the other lifecycle hooks in `CheckpointCoordinator` are documented. 3. **Chain-stop assertion** — the test proves FAILED and the manager notification, but not that the periodic chain actually stops afterwards. Asserting that no further trigger is scheduled once the coordinator is FAILED would cover the second half of the fix's intent. None of these should hold up merging; whether to rerun CI once more or merge with the documented infra-flake context is a call for a maintainer. Happy to take another look if you push a follow-up. <!-- streview-comment:1425 --> -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
