Rangsh commented on PR #12081: URL: https://github.com/apache/seatunnel/pull/12081#issuecomment-6008475917
@SEZ9 Thanks — agreed on the flake read, and glad the `notifyCheckpointMonitor` regression is closed out. **Restating the `SplitClusterFaultToleranceIT` options, with the trade-offs as I see them:** Context: `SplitClusterFaultToleranceIT#testStreamJobRestoreInAllNodeDown` kills the whole 4-node cluster, restores it, and asserts the job reaches `CANCELED` after `cancelJob()`. During that final cancellation an IMap bookkeeping write can hit the 60s store-write timeout because the WAL worker stalls under the cancel storm on the test's `file:///` shared-path setup. The stall is pre-existing; what this PR changes is that fail-loud `FileMapStore` surfaces it as an explicit failure, so the job ends `FAILED` (observed as `UNKNOWABLE` by the client) instead of eventually reaching `CANCELED`. **(a) Keep fail-loud as-is; track the cancel-path slow write separately.** Trade-off: preserves the PR's core safety property (a WAL durability failure is never silently swallowed) and keeps this already-large PR scoped — the underlying stall stays tracked where it belongs (#12492 worker hardening). The cost: until the stall is fixed, a rare cancel-path timeout can show up as a red/flaky IT, and a job whose cancel-path bookkeeping write times out ends `FAILED` rather than `CANCELED` — a user-visible behavior change on that path. **(b) Isolate store-write timeouts during cancellation in this PR.** Concretely: when the job is already `CANCELING`, downgrade a store-write failure/timeout for bookkeeping writes to a WARN instead of failing the task — cancel-path-only isolation, analogous to the `notifyCheckpointMonitor` scoping we just did. Trade-off: the test outcome becomes stable and matches dev's observable behavior. The cost: it widens this PR into the cancellation path of the vertex state machine — a sensitive area that deserves its own review cycle; it risks re-introducing exactly the too-broad-catch class of bug we just closed unless scoped precisely (only when already `CANCELING`, only bookkeeping writes, never data-carrying checkpoint writes); and it partially undoes the fail-loud philosophy by papering over the underlying stall rather than surfacing it. My lean is (a): the stall is pre-existing and tracked, and a precisely-scoped cancellation-path carve-out is safer to design and review in its own PR. Happy to implement (b) here if you prefer. **The earlier review points on `ab04db33a`, one line each:** 1a. **Untimed `RequestFuture.get()` unbounded blocking — fixed in this commit.** Method-level javadoc states it blocks indefinitely, exists for `Future` contract compliance only, and must not be used where an unbounded wait is unacceptable; no production caller currently invokes it (`RequestFuture.java` L57–68). 1b. **Timed `RequestFuture.get(timeout, unit)` semantics / `TimeoutException` / javadoc — fixed in this commit.** It declares `@throws TimeoutException` in its method-level javadoc and throws it on expiry instead of returning `false` (`RequestFuture.java` L70–90); both production callers were rewritten to that contract. 2. **`WALWorkHandler` worker-death / writer-reuse-after-failure — fixed in this commit.** `writer.write()` is wrapped in `catch (Exception)` so the append result is always published and the sole consumer cannot die on an append failure (L109–115); 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 (L99–105). Intentionally unchanged: an `Error` escaping `writer.write()` can still kill the worker — tracked in #12492 as agreed. 3. **Sequential full-timeout waits in `batchQueryExecuteFailsStatus` — fixed in this commit.** One shared `deadlineNanos` is computed before the loop and each entry waits only `Math.max(0, deadline - now)`, bounding the whole batch to one `writDataTimeoutMilliseconds` instead of N times it (`IMapFileStorage.java` L354–377); the bounded total wait is asserted by `IMapFileStorageBatchDeadlineTest`. 4. **ERROR stack on every timed-out wait in `queryExecuteStatus` — fixed in this commit.** Timeouts log a single-line WARN with requestId/elapsed/limit, stack trace at DEBUG; ERROR + stack is reserved for the unexpected `catch (Exception)` branch (`IMapFileStorage.java` L336–345). **On `HdfsWriterFlushSyncPathTest`:** at `ab04db33a` I can't find a test by that name; the Mockito-based tests in the tree are `HdfsWriterFlushCallCountTest` (added in `70e40b40e`, "Assert HdfsWriter.flush uses exactly one hsync path") and `WALWorkHandlerSurvivabilityTest`. `org.mockito:mockito-junit-jupiter` is already declared in test scope in `imap-storage-file/pom.xml` (L76–80, comment naming both tests) — no pom change needed. Once you've weighed in on (a)/(b) I'll act on it right away — thanks again for the careful final pass! -- 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]
