SEZ9 commented on PR #12081: URL: https://github.com/apache/seatunnel/pull/12081#issuecomment-5564339376
Thanks @DanielLeens for taking the time to separate the two questions rather than restating the earlier approval — that framing matches how I'd like to land this. On the first bucket (`RequestFuture` completion/success semantics and the `WALWorkHandler` catch-widening), I agree these stand on their own merits, but I want to close out the points from my earlier review before we call them done: - **`RequestFuture.get()` unbounded wait / semantic change (F1, F3, F7).** Moving the untimed `get()` from a 1-second-capped wait to an unbounded block, and the timed `get()` from "return false" to `TimeoutException`, is a behavioral change for every caller. Please confirm there is no remaining caller relying on the old untimed `get()` returning, and add the new timeout contract to the `get(timeout, unit)` method Javadoc itself, not just the class-level comment. - **Worker survivability (F2, F4).** Widening the catch is the right direction, but as written the single disruptor consumer can still die if `executeResponse()` throws, and after an arbitrary write failure the handler continues with the same writer, which risks a torn record mid-file. I'd like either a guard around `executeResponse()` plus a writer reset/reopen on non-`IOException` failures, or a short note on why those paths are considered unreachable. - **Batch wait behavior (F6, F8).** `batchQueryExecuteFailsStatus` now waits the full configured timeout per entry sequentially, so a stuck worker can block `storeAll`/`deleteAll` for N × the timeout. Please consider a shared deadline across the batch, and downgrade the per-entry timeout logging so we don't emit a full stack trace at ERROR for each timed-out wait. On the second bucket, I'll stay within what I reviewed: `HdfsWriterFlushSyncPathTest` verifies method-call-count parity, and I agree with you that this should not be described as evidence of disk-sync-count parity or of resolving the reported variance — please adjust the PR description/test naming accordingly. Separately (F5), the test introduces Mockito but I don't see a `pom.xml` change adding the test dependency to the `imap-storage-file` module; please add it or point me to where it's inherited from. Once the items above are addressed I'm happy to re-review the `RequestFuture`/`WALWorkHandler` portion independently of the benchmark question. <!-- streview-comment:858 --> -- 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]
