SEZ9 commented on PR #12081: URL: https://github.com/apache/seatunnel/pull/12081#issuecomment-5825984599
Thanks for the detailed follow-up. Going through the points: **F3 (timed `get()` → `TimeoutException`)** — Makes sense. If `queryExecuteStatus(long, long)` and `batchQueryExecuteFailsStatus` are the only callers and both catch `TimeoutException` and resolve to `false`, the observable timeout behavior is preserved. I'll confirm both call sites in the diff on the final pass. **F5 (Mockito dependency)** — An explicit test-scope `org.mockito:mockito-junit-jupiter` entry in `imap-storage-file/pom.xml` is what I was after. One small note: you mention the pom comment references `HdfsWriterFlushCallCountTest` / `WALWorkHandlerSurvivabilityTest`, while the finding was raised against `HdfsWriterFlushSyncPathTest`. Please make sure the comment names the tests that actually exist in the module. **F8 (timeout logging)** — The split you describe (single `log.warn` with requestId/elapsed/limit, stack trace only at debug, `log.error` reserved for the generic `catch (Exception e)` path) is exactly what I wanted. I'll mark it resolved once I've seen it in the diff. **F1 / F2 (unbounded `get()` and worker death when `executeResponse()` throws)** — Understood that the `Error`/dead-worker hardening is being deferred and that no tracking issue exists yet. Since it isn't tracked anywhere, could you open the issue and link it here so it doesn't fall through the cracks? If `WALWorkHandlerSurvivabilityTest` already covers part of the worker-survival case, a quick note on what it asserts would help me scope what's left. **Still open:** - **F4 (reusing the writer after a write failure):** No update yet — please confirm whether the handler now discards/reopens the writer on failure, or explain why a torn record mid-file is acceptable. - **F6 (sequential full-timeout waits in `batchQueryExecuteFailsStatus`):** You mention a shared `deadlineNanos`. If that means the whole batch is bounded by a single timeout rather than N × `writDataTimeoutMilliseconds`, please say so explicitly and I'll verify. - **F7 (method-level Javadoc on `get(timeout, unit)`):** Please add a short note on the method itself that it throws `TimeoutException`, not just at class level. Once those are answered/pushed I'll do the final pass. <!-- streview-comment:1306 --> -- 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]
