SEZ9 commented on PR #12081: URL: https://github.com/apache/seatunnel/pull/12081#issuecomment-5594903816
Thanks @DanielLeens — the independent re-trace of the `RequestFuture`/`WALWorkHandler`/`HdfsWriter.flush()`/`IMapFileStorage` chain against `5b691c615` is very helpful. Mapping it onto my earlier points, here is where I think each one stands: - **Worker survivability (F2, F4):** The described change (`WALWorkHandler` catching `Exception`, with `executeResponse()` separately guarded) is the right shape for F2. Since this round is described as a 3-line test-only change on top of `f08bdf0ac`, could you point me at the commit or hunk where that `WALWorkHandler` change actually lands so I can verify it? F4 is still open either way: after an arbitrary write failure the handler keeps reusing the same writer, so a partially written record can remain mid-file. I'd like either a writer reset/reopen on failure or a short justification for why the WAL reader tolerates a torn trailing record. - **Batch deadline (F6):** A single shared deadline across `batchQueryExecuteFailsStatus` would address the `N × timeout` concern. Same request as above: please point to the `IMapFileStorage` change, and confirm the deadline is computed once before the loop and the remaining time passed to each timed `get(timeout, unit)` is clamped at zero rather than going negative. - **`RequestFuture.get()` semantics (F1, F3, F7):** Throwing `TimeoutException` from the timed `get()` is the correct `Future` contract. The untimed `get()` moving from a 1-second cap to an unbounded block is still a behaviour change I'd like handled explicitly: either confirm no production call site uses the bare `get()`, or document on the method itself (not only at class level) that it blocks indefinitely and callers should use the timed variant. A method-level Javadoc line describing the `TimeoutException` behaviour on the timed `get(timeout, unit)` would close F7 as well. - **Timeout logging (F8):** Each timed-out wait in `queryExecuteStatus` now logs a full stack trace at ERROR, so a stuck worker emits one per entry. I'd prefer a single-line WARN with the key and elapsed time, with the stack trace at DEBUG. - **Mockito dependency (F5):** `HdfsWriterFlushSyncPathTest` uses Mockito, but I don't see a `pom.xml` change for the `imap-storage-file` module in this PR. Please confirm whether Mockito is already available transitively in that module's test scope, or add the dependency. Once F4, F5 and the method-level Javadoc are addressed and the F2/F6 changes and deadline clamping are confirmed against the diff, I'm happy to do a final pass. <!-- streview-comment:911 --> -- 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]
