SEZ9 commented on PR #12081: URL: https://github.com/apache/seatunnel/pull/12081#issuecomment-5807093777
Thanks for the detailed pass on `7f69d7f69f6`. Where things stand from my side: - **F1 / F3 (`RequestFuture.get()`):** The untimed overload having no production caller, with `queryExecuteStatus` and the batch path using the timed overload, addresses the unbounded-block half. The other half of F3 is the timed `get()` contract change (throwing `TimeoutException` instead of returning `false`) — could you point me to where existing callers were updated for that, or confirm none depended on the old return value? - **F2 / F4:** The widened `catch (Exception e)` in `walEvent()`, the `try/catch` in `executeResponse()`, and the fail-closed `appendBlockedAfterWriteFailure` flag are the behaviour I was after. I'll confirm against the diff on `7f69d7f69f6` before approving. - **F6 / F7:** Same — the shared `deadlineNanos` in the batch path and the `TimeoutException` contract documented on the timed `get()` sound right; I'll verify in the diff. Two items from the original list aren't covered in the thread yet: - **F5 (Mockito in the new test):** Is there a build change in this PR adding the Mockito test dependency to the module, or does it already come in transitively? If the latter, a quick pointer to where it comes from is enough. - **F8 (logging on write timeout):** Is the `TimeoutException` in `queryExecuteStatus` still logged at ERROR with a full stack trace? Since a timeout is an expected outcome now, WARN without the trace (or a single-line message) would keep logs readable under a stuck worker. On the residual gap — `catch (Exception e)` still lets an `Error` (e.g. `OutOfMemoryError`) escape and kill the worker thread, after which nothing detects the dead worker and every append fails closed — I agree it's out of scope for this PR. Please open a follow-up issue and link it here so we don't lose track. Once F3 (timed half), F5 and F8 are answered I'll do a final check of the diff and approve. <!-- streview-comment:1287 --> -- 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]
