Rangsh commented on PR #12081: URL: https://github.com/apache/seatunnel/pull/12081#issuecomment-6594921283
@SEZ9 thanks. Answers against `ef99d22d1`: **F1 field name.** I would leave `writDataTimeoutMilliseconds` as-is in this PR. The missing `e` is pre-existing on that private field (added in #4059); the public config key is already `writeDataTimeoutMilliseconds`. Renaming the field here would not change behavior, so I would keep it for a follow-up. **F2 / F4.** Covered in [this comment](https://github.com/apache/seatunnel/pull/12081#issuecomment-6558601142) and on this head in [`WALWorkHandler`](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#L39-L140): - The 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 touching the writer ([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`, and `executeResponse` has its own try/catch ([L110-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#L110-L140)). **F5.** [`mockito-junit-jupiter` in `imap-storage-file/pom.xml`](https://github.com/apache/seatunnel/blob/ef99d22d1afec8dc6ce094636a3eeb26910cdd03/seatunnel-engine/seatunnel-engine-storage/imap-storage-plugins/imap-storage-file/pom.xml#L81-L90) is already an explicit test dependency, so no further pom change is needed. **F6.** The shared deadline is computed once 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 already 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.** `queryExecuteStatus` 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). Let me know if these look good to you, 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]
