SEZ9 commented on PR #12081: URL: https://github.com/apache/seatunnel/pull/12081#issuecomment-5611876264
Thanks @DanielLeens — a from-scratch re-trace of the `RequestFuture` / `WALWorkHandler` / `IMapFileStorage` / `DefaultReader` chain against `64b0ec85a` is a very useful independent data point, and the `5b691c61..64b0ec85` stat breakdown you posted (four files, comment/Javadoc-only changes in `WALWorkHandler.java` and `RequestFuture.java`, plus the new `DefaultReaderTornTrailingRecordTest.java`) is a helpful map for the rest of us. On F4: your reading of `DefaultReader.readData()` — whole-file read, length decoded from the 12-byte metadata prefix, and the `if (startIndex + dataLength > datas.length) break;` guard before any payload bytes are touched — is exactly the reader-side verification this finding needed. Two things before I can treat F4 as closed, though: - Your comment appears to have been cut off mid-sentence right after "This is exactly the" — could you post the remainder? - F4 is about a torn record in the *middle* of the file, not only at the tail. The `break` you describe only protects a trailing torn record. Does the new test (or the reader logic) cover a truncated record that is followed by further bytes? If not, I'd like to keep F4 open until that case is either covered or explicitly argued to be unreachable. For the remaining items, a short status from your trace would be great: - **F6** (`batchQueryExecuteFailsStatus` in `IMapFileStorage.java`): the `+31/-9` there is the only non-trivial code change in this round. Does it now bound the total wait for `storeAll`/`deleteAll` across N entries, or does it still wait up to `writDataTimeoutMilliseconds` per entry sequentially? - **F7** (`RequestFuture.java`, +3/-3 Javadoc): is the `TimeoutException` contract now documented on `get(timeout, unit)` itself, not just at class level? - **F8** (`queryExecuteStatus` in `IMapFileStorage.java`): is a timed-out wait still logging a full `TimeoutException` stack trace at ERROR, or has that been trimmed/downgraded? - **F1 / F2 / F3**: you didn't mention these — were they already resolved in your trace, or out of scope for this round? One line each is enough. - **F5**: I still don't see a `pom.xml` change adding a Mockito test dependency to the `imap-storage-file` module for `HdfsWriterFlushSyncPathTest`. If it's inherited from a parent, please point to where; otherwise this needs a pom change. Once the truncated portion and those statuses are in, I'll do my own final pass. <!-- streview-comment:938 --> -- 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]
