DanielLeens commented on PR #12081: URL: https://github.com/apache/seatunnel/pull/12081#issuecomment-5846261922
Thanks for the careful tracking, @SEZ9 — let me close out the remaining items. **F4 (writer reuse after a write failure):** The handler does not discard/reopen the writer. After any APPEND write failure it sets a sticky `appendBlockedAfterWriteFailure` flag (`WALWorkHandler.java:63`) and every subsequent APPEND fails closed — `executeResponse(requestId, false)` without touching the writer again — for the remaining lifetime of the handler; only a process restart clears it. The class Javadoc explains why re-opening isn't safe: a blind `fs.create` reopen would truncate the fixed `wal.txt` path, and `DefaultReader` can't resync past a torn frame in the *middle* of the file (it stops once a length prefix claims more bytes than remain), so continuing to write risks placing a complete frame after a partial one. Fail-closing instead leaves at worst a trailing incomplete record, with every record before it still recoverable — that trade-off is exercised directly by the two new reader tests, `DefaultReaderTornTrailingRecordTest` (trailing torn record, recoverabl e) and `DefaultReaderTornMidFileRecordTest` (mid-file torn record, the case fail-close is specifically avoiding). `WALWorkHandlerSurvivabilityTest` is the test that pins the fail-close behavior itself, so to answer your scoping question on F1/F2 in the same breath: it has two cases. One injects a non-`IOException` from `writer.write(...)` and asserts the worker doesn't die and the *next* APPEND fails closed without the writer being called again. The other injects the failure at `hsync()` inside a real `HdfsWriter` — so the failure happens during flush/sync, not just the initial write — and additionally verifies `hsync()` was actually invoked once before fail-close tripped. Both assert the sticky flag and that the writer is touched exactly once across the two events. What it does *not* cover is the residual gap I flagged last round: an `Error` (e.g. `OutOfMemoryError`) escaping `writer.write()` still isn't caught by the `catch (Exception e)`, so it would still kill the worker thread with nothing to detect it. That's the one still worth a tracking issue — I haven't found an existing one, so I'll open it and link it back here rather than asking either of you to chase it down. **F6 (shared batch deadline):** Confirmed explicitly, since you asked for it in plain terms — yes, `batchQueryExecuteFailsStatus` computes `deadlineNanos` once before the loop, and each entry's wait is `Math.max(0, deadlineNanos - now)`, skipping the wait entirely once expired. The whole batch is bounded by one shared timeout, not N × `writDataTimeoutMilliseconds`. **F7 (method-level Javadoc on the timed `get`):** Already there in the current diff — `RequestFuture.get(long, TimeUnit)` has a full method Javadoc including `@throws TimeoutException if the wait times out before completion`, so nothing further needed on that one. **F5 (pom comment test names):** I went back and checked the full file list in this diff rather than relying on memory — the pom comment already names `HdfsWriterFlushCallCountTest` and `WALWorkHandlerSurvivabilityTest`, and those are exactly the two Mockito-based test classes that exist in the module. There's no `HdfsWriterFlushSyncPathTest` anywhere in the current diff; I believe that name is from an earlier point in this thread that never shipped under that name, rather than something the current comment is out of sync with. I don't think there's anything to change here. That leaves F3 and F8 as you already noted — matching what I described, ready for you to confirm directly against the diff. Thanks again for staying this thorough through every round; happy to do the final pass together once you've had a chance to look. === END REPLY === -- 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]
