Rangsh commented on PR #12081: URL: https://github.com/apache/seatunnel/pull/12081#issuecomment-6623602753
@SEZ9 thanks. I've addressed all the remaining items below, with direct links to the changes on `ef99d22d1`. **F1.** The untimed `get()` cannot be removed because `Future` requires it. It now delegates to the timed `get` with the configured timeout, so a dead WAL worker or a never-dispatched event cannot block forever: [`RequestFuture.java` L84-L108](https://github.com/apache/seatunnel/blob/ef99d22d1afec8dc6ce094636a3eeb26910cdd03/seatunnel-engine/seatunnel-engine-storage/imap-storage-plugins/imap-storage-file/src/main/java/org/apache/seatunnel/engine/imap/storage/file/future/RequestFuture.java#L84-L108). **F2 / F4.** Confirmed in [`WALWorkHandler.java`](https://github.com/apache/seatunnel/blob/ef99d22d1afec8dc6ce094636a3eeb26910cdd03/seatunnel-engine/seatunnel-engine-storage/imap-storage-plugins/imap-storage-file/src/main/java/org/apache/seatunnel/engine/imap/storage/file/disruptor/WALWorkHandler.java#L95-L140): - The failure flag stays set for the handler lifetime ([L57-L72](https://github.com/apache/seatunnel/blob/ef99d22d1afec8dc6ce094636a3eeb26910cdd03/seatunnel-engine/seatunnel-engine-storage/imap-storage-plugins/imap-storage-file/src/main/java/org/apache/seatunnel/engine/imap/storage/file/disruptor/WALWorkHandler.java#L57-L72)). - Later APPEND events return `done(false)` before the writer is touched ([L95-L104](https://github.com/apache/seatunnel/blob/ef99d22d1afec8dc6ce094636a3eeb26910cdd03/seatunnel-engine/seatunnel-engine-storage/imap-storage-plugins/imap-storage-file/src/main/java/org/apache/seatunnel/engine/imap/storage/file/disruptor/WALWorkHandler.java#L95-L104)). - `writer.write` is caught as `Exception` ([L110-L116](https://github.com/apache/seatunnel/blob/ef99d22d1afec8dc6ce094636a3eeb26910cdd03/seatunnel-engine/seatunnel-engine-storage/imap-storage-plugins/imap-storage-file/src/main/java/org/apache/seatunnel/engine/imap/storage/file/disruptor/WALWorkHandler.java#L110-L116)). - `executeResponse()` is on that guarded path and has its own try/catch around `RequestFutureCache.get` and `future.done` ([L117-L140](https://github.com/apache/seatunnel/blob/ef99d22d1afec8dc6ce094636a3eeb26910cdd03/seatunnel-engine/seatunnel-engine-storage/imap-storage-plugins/imap-storage-file/src/main/java/org/apache/seatunnel/engine/imap/storage/file/disruptor/WALWorkHandler.java#L117-L140)). A future already removed by the caller-side timeout is logged and cannot take down the single disruptor worker. Same point is in [comment 6558601142](https://github.com/apache/seatunnel/pull/12081#issuecomment-6558601142). **F3.** Both callers in this module use the timed `get` and treat timeout as a failed write: [`IMapFileStorage.java` L373-L437](https://github.com/apache/seatunnel/blob/ef99d22d1afec8dc6ce094636a3eeb26910cdd03/seatunnel-engine/seatunnel-engine-storage/imap-storage-plugins/imap-storage-file/src/main/java/org/apache/seatunnel/engine/imap/storage/file/IMapFileStorage.java#L373-L437). **F5.** `mockito-junit-jupiter` is an explicit test dependency of `imap-storage-file`, added in this PR: [`pom.xml` L81-L90](https://github.com/apache/seatunnel/blob/ef99d22d1afec8dc6ce094636a3eeb26910cdd03/seatunnel-engine/seatunnel-engine-storage/imap-storage-plugins/imap-storage-file/pom.xml#L81-L90). The flush test is [`HdfsWriterFlushCallCountTest`](https://github.com/apache/seatunnel/blob/ef99d22d1afec8dc6ce094636a3eeb26910cdd03/seatunnel-engine/seatunnel-engine-storage/imap-storage-plugins/imap-storage-file/src/test/java/org/apache/seatunnel/engine/imap/storage/file/wal/writer/HdfsWriterFlushCallCountTest.java) (renamed from `HdfsWriterFlushSyncPathTest`). Mockito also comes from the root POM test dependencies. **F6.** One shared deadline is computed before the loop. Each entry waits only for the remaining budget, and `get(0, NANOSECONDS)` still harvests an already-completed future: [`IMapFileStorage.java` L394-L417](https://github.com/apache/seatunnel/blob/ef99d22d1afec8dc6ce094636a3eeb26910cdd03/seatunnel-engine/seatunnel-engine-storage/imap-storage-plugins/imap-storage-file/src/main/java/org/apache/seatunnel/engine/imap/storage/file/IMapFileStorage.java#L394-L417). **F7.** Method-level javadoc states that `get(timeout, unit)` throws `TimeoutException` instead of returning `false`: [`RequestFuture.java` L110-L130](https://github.com/apache/seatunnel/blob/ef99d22d1afec8dc6ce094636a3eeb26910cdd03/seatunnel-engine/seatunnel-engine-storage/imap-storage-plugins/imap-storage-file/src/main/java/org/apache/seatunnel/engine/imap/storage/file/future/RequestFuture.java#L110-L130). **F8.** A timed-out wait logs one WARN line with the request id and timeout. The stack trace is DEBUG only: [`IMapFileStorage.java` L378-L385](https://github.com/apache/seatunnel/blob/ef99d22d1afec8dc6ce094636a3eeb26910cdd03/seatunnel-engine/seatunnel-engine-storage/imap-storage-plugins/imap-storage-file/src/main/java/org/apache/seatunnel/engine/imap/storage/file/IMapFileStorage.java#L378-L385). The batch path does the same at [L418-L428](https://github.com/apache/seatunnel/blob/ef99d22d1afec8dc6ce094636a3eeb26910cdd03/seatunnel-engine/seatunnel-engine-storage/imap-storage-plugins/imap-storage-file/src/main/java/org/apache/seatunnel/engine/imap/storage/file/IMapFileStorage.java#L418-L428). Nothing here is deferred except the pre-existing private-field typo. Let me know if these look good, and I am ready for your final pass. -- 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]
