Rangsh commented on PR #12081: URL: https://github.com/apache/seatunnel/pull/12081#issuecomment-6029505221
@SEZ9 Thanks for the decision — going with **(a)**, and I will not implement (b) here. Both asks are done: 1. #12492 now records the user-visible change explicitly (new "User-visible behavior change to be aware of" section: cancel-path bookkeeping write timeout → job ends `FAILED`/`UNKNOWABLE` instead of `CANCELED`, kept per the fail-loud decision), and this PR's description has a one-line pointer to #12492 in the out-of-scope list. 2. `SplitClusterFaultToleranceIT#testStreamJobRestoreInAllNodeDown` stays exactly as-is — no retry wrapper, no disable. Agreed: if that path goes red again, it should be visible. Reposting the full one-line recap (the previous one was cut off after 1a — sorry about that): 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)` `TimeoutException` contract documented on the method itself — fixed in this commit.** The `@throws TimeoutException` javadoc is on the method, not just the class, and it throws on expiry instead of returning `false` (`RequestFuture.java` L70–90); both production callers use the timed overload. 2. **Single disruptor worker surviving an `executeResponse()` throw — fixed in this commit.** `executeResponse()` is wrapped in its own `try/catch (Exception)` so a response-publishing failure (including the timeout+remove race where the future is already gone from the cache) logs and returns instead of killing the sole consumer (`WALWorkHandler.java` L129–140). 3. **Writer reuse after an arbitrary write failure (torn-record risk) — intentionally fail-closed, not reused.** After any APPEND write failure the sticky `appendBlockedAfterWriteFailure` flag rejects further APPENDs without touching the stream, so the writer is never reused on a possibly-torn stream; recovery requires process restart (`WALWorkHandler.java` L99–105, class javadoc L39–52). Intentionally unchanged: an `Error` escaping `writer.write()` can still kill the worker — tracked in #12492. 4. **Mockito test dependency for `HdfsWriterFlushSyncPathTest` (renamed `HdfsWriterFlushCallCountTest` in `26ce5cb26`) — fixed in this commit.** `org.mockito:mockito-junit-jupiter` is declared in test scope in `imap-storage-file/pom.xml` (L76–80); no pom change is still needed. 5. **`batchQueryExecuteFailsStatus` waiting the full timeout per entry sequentially — 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 × it (`IMapFileStorage.java` L354–377); asserted by `IMapFileStorageBatchDeadlineTest`. 6. **Full stack trace at ERROR 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). Ready for your final pass — thanks again! -- 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]
