SEZ9 commented on PR #12081: URL: https://github.com/apache/seatunnel/pull/12081#issuecomment-5787992822
Thanks for the consolidated rationale, @Rangsh — that saves a lot of back-and-forth. To answer directly: based on what you describe, I don't need further commits on this PR for F2, F4, F6 and F7, subject to my final pass on `7f69d7f69`. A few points so that pass can be quick: 1. **F1 / F3 (untimed `get()`)** — Understood that the production callers only use the timed `get(timeout, unit)` and are bounded by `writDataTimeoutMilliseconds` / the shared batch deadline. The residual is the untimed `get()` itself, which is still unbounded if the WAL worker dies via an `Error`. I'm fine handling that as a separate follow-up rather than in this PR — please open it (or tell me if you'd prefer I do), and consider a one-line note on the untimed `get()` Javadoc that callers should prefer the timed variant. 2. **F2** — Could you confirm whether the `try/catch` around `executeResponse()` also covers `Error`? If not, that's acceptable for this PR, but it belongs in the same follow-up as F1. 3. **F4** — Fail-close via `appendBlockedAfterWriteFailure` is the right call; no change requested. If there is a unit test asserting APPENDs are rejected after a write/flush failure without touching the stream, point me at it and I'll check it in the final pass. 4. **F6 / F7** — Shared `deadlineNanos` and the method-level `TimeoutException` Javadoc address these as summarized; nothing further. 5. **F5 / F8** — Rather than rely on memory, could you point me at the `pom.xml` change adding the Mockito test dependency (F5) and confirm how a timed-out write in `queryExecuteStatus` is logged now (F8), so I can tie both to `7f69d7f69` explicitly? Once I have those pointers I'll do the synced-head final pass on `7f69d7f69`. <!-- streview-comment:1246 --> -- 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]
