SEZ9 commented on PR #12081: URL: https://github.com/apache/seatunnel/pull/12081#issuecomment-6513897688
Thanks for the rebase to `8f2f6fa986b04ef3f96315dcf722a282357d744d` on top of `dev` at `5598ed7f3a88602032a18b617cb21c688aacf965`. Going back over the earlier points: **F2 / F4 (`WALWorkHandler`), F6 (`IMapFileStorage`)** – The described approach (catching `Exception` around the write and response publishing so the single disruptor worker survives, a sticky append block after a failed append so later requests complete `false` instead of writing past a possibly torn frame, and one shared wait deadline for `storeAll`/`deleteAll`) is what I was hoping for. I haven't verified it against the diff yet; could you point me to the relevant hunks so I can confirm before closing these? **F1 / F3 (`RequestFuture`)** – Keeping these together since they overlap. The timed `get(timeout, unit)` throwing `TimeoutException` with callers consuming the actual `Boolean` result sounds consistent. What I still need to understand is the untimed `get()`: is it no longer called from the storage path, or is it bounded somehow? If a request is never dispatched (for example, the worker is blocked and the caller-side timeout has already removed the future), nothing should be able to wait forever. **F5** – I don't see a `pom.xml` change adding a Mockito test dependency for the `imap-storage-file` module. If `HdfsWriterFlushSyncPathTest` relies on Mockito arriving transitively, please add it explicitly in test scope so an unrelated dependency change can't break the test. **F7** – Please document the "throws `TimeoutException` on timeout, otherwise returns the `Boolean` result" contract on `RequestFuture.get(timeout, unit)` itself, not only in the class-level Javadoc. **F8** – In `queryExecuteStatus`, a timeout is an expected operational outcome rather than a bug. Please log it at WARN with the key/request id and without the full stack trace, reserving ERROR for unexpected exceptions. Once F5/F7/F8 are in and F1/F3 is clarified, I'm happy to take another pass. <!-- streview-comment:2136 --> -- 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]
