Rangsh commented on PR #12081: URL: https://github.com/apache/seatunnel/pull/12081#issuecomment-5987300542
@SEZ9 The `notifyCheckpointMonitor` fix is pushed in `ab04db33a` — both `CheckpointCoordinator` and `CheckpointManager` now catch only `IMapStorageException` (the auxiliary monitor-map durability failure from the fail-loud `FileMapStore`), while `HazelcastInstanceNotActiveException` and other lifecycle signals propagate exactly as on `dev`. Unit tests cover both paths: the isolated `IMapStorageException` case and the propagated `HazelcastInstanceNotActiveException` case (`CheckpointCoordinatorTest#testCompletePendingCheckpointPropagatesNodeShutdownExceptionFromMonitor`, `CheckpointManagerTest#testMonitorNodeShutdownExceptionShouldPropagate` / `#testMonitorStorageExceptionShouldBeIsolated`). `engine-v2-it` has been re-dispatched on this head for JDK 8 and JDK 11; I'll report back with the `CheckpointCoordinatorFailoverIT#testBatchJobCompletesAfterMasterFailoverDuringCloseHandshake` result on both. On the five earlier review points — I believe each is already addressed in the current diff, and I'd like to tie each one to its location so we can close them out together. If any of them still looks open to you after checking these, I'm happy to keep working on them: 1. **`RequestFuture.get()` unbounded wait / untimed-timed semantics / `TimeoutException` contract** — the untimed `get()` carries a method-level Javadoc stating it blocks indefinitely, that it exists for `Future` contract compliance only, and that callers should prefer the timed variant (`RequestFuture.java`, lines 57–63); no production caller invokes it. The timed `get(timeout, unit)` documents the `@throws TimeoutException` contract on the method itself (lines 70–90), and both production callers (`queryExecuteStatus` and `batchQueryExecuteFailsStatus`) were rewritten to the `TimeoutException` contract in this diff. 2. **`WALWorkHandler` worker death / writer reuse after failure** — `executeResponse()` is wrapped in its own `try/catch (Exception)` so response publishing cannot kill the sole disruptor consumer (`WALWorkHandler.java`, lines 129–140); after any write failure the sticky `appendBlockedAfterWriteFailure` flag fail-closes further APPENDs without touching the stream, so the writer is never reused after a failure. Covered by `WALWorkHandlerSurvivabilityTest`, `DefaultReaderTornTrailingRecordTest` and `DefaultReaderTornMidFileRecordTest`. 3. **Sequential full-timeout waits in `batchQueryExecuteFailsStatus`** — the batch now computes a single shared `deadlineNanos` once before the loop, and each entry waits `Math.max(0, deadlineNanos - now)` so the whole batch is bounded by one `writDataTimeoutMilliseconds` rather than N × (`IMapFileStorage.java`, lines 352–398). `IMapFileStorageBatchDeadlineTest` asserts the bounded total wait directly. 4. **Mockito test dependency** — declared explicitly in test scope in `imap-storage-file/pom.xml` (`org.mockito:mockito-junit-jupiter`). 5. **ERROR stack trace on every timed-out wait** — timeouts in `queryExecuteStatus` now log a single-line WARN with requestId/elapsed/limit, with the stack trace at DEBUG; ERROR + stack trace is reserved for the unexpected `catch (Exception)` branch (`IMapFileStorage.java`, lines 331–350). The `Error`-escapes-`writer.write()` / dead-worker hardening remains tracked in #12492 as agreed. Thanks again for the detailed final-pass guidance — happy to adjust any of the above if you see it differently. -- 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]
