Rangsh commented on PR #12081: URL: https://github.com/apache/seatunnel/pull/12081#issuecomment-5653099419
@DanielLeens thanks for the from-scratch re-review and the Approve — especially for independently confirming the `notifyCheckpointMonitor` coverage across every call site and for pinning the `Throwable` vs `Exception` inconsistency against this PR's own `WALWorkHandler` precedent. Addressed the non-blocking items from review `5190463250`: 1. **New Issue 1 (Medium)** — narrowed both `notifyCheckpointMonitor` helpers (`CheckpointCoordinator` / `CheckpointManager`) from `catch (Throwable t)` to `catch (Exception e)`, with a short comment pointing at the `WALWorkHandler.onEvent()` Exception-only boundary so a genuine `Error` (e.g. `OutOfMemoryError`) still propagates. Commit: `3d9f80f04`. Re-ran `CheckpointCoordinatorTest#testCompletePendingCheckpointContinuesWhenMonitorThrows` successfully after the change (the `RuntimeException` / `IMapStorageException` path is unchanged). 2. **New Issue 2 (Low)** — leaving the near-duplicate helpers as-is for this PR's scope, as you suggested (future cleanup / shared utility). 3. **New Issue 3 (CI / divergence)** — merged current `upstream/dev` into this branch (`7f69d7f69`). Head is now **0 behind** `dev` (was 125). Please take another look when convenient; CI should re-run on the synced head. -- 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]
