Rangsh commented on PR #12493: URL: https://github.com/apache/seatunnel/pull/12493#issuecomment-5856438922
@DanielLeens Thanks for the thorough review — I verified each issue against the code and addressed them in 21501f090: 1. **(Medium) InterruptedException / commonPool interrupt leak** — Confirmed: `startSavepoint()` is invoked via `CheckpointManager.triggerSavePoints()`'s `parallelStream()` on `ForkJoinPool.commonPool()`. Removed `Thread.currentThread().interrupt()`; the shared future already carries the failure to callers, so re-interrupting is unnecessary and unsafe on a shared-pool worker. 2. **(Low) Missing log on interrupted drain** — Added `LOG.warn` with job/pipeline id when the drain is aborted by interruption. 3. **(Low) Defensive `createPendingCheckpoint` guard failure mode** — Wrapped create/start in `tryTriggerPendingCheckpoint` with catch → `LOG.error` + `scheduleTriggerPendingCheckpoint(..., 500L)` so a future regression fails loud and keeps the periodic cadence alive instead of being silently dropped by the scheduler. 4. **(Low) `savepointPendingCheckpoint` write outside lock** — Moved the assignment into a `synchronized (lock)` block. Re-ran the four part-1 UTs (`testTryTriggerNotBlockedBySavepointDrain` / `testSavepointDrainGateReArmsWhenPendingCounterZero` / `testConcurrentStartSavepointSharesOneRequest` / `testSavepointDrainInterruptClearsGateAndAllowsTrigger`): **Tests run: 4, Failures: 0**. Would appreciate another look when you have a chance. -- 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]
