Rangsh commented on PR #12081:
URL: https://github.com/apache/seatunnel/pull/12081#issuecomment-5568946298
@SEZ9 @DanielLeens @nzw921rx thank you for the detailed follow-up —
especially for separating the correctness fixes from the still-open #12058 CV
investigation.
Pushed `26ce5cb26` on `improve/zeta-checkpoint-state-store-latency-12058`
and updated the PR title/description accordingly (`Related to #12058`, softened
the root-cause claim to a working hypothesis, and clarified that merge must not
auto-close #12058).
### Addressing @SEZ9 (F1–F8)
1. **`RequestFuture.get()` / timed contract (F1, F3, F7)**
- Re-confirmed production callers: only
`IMapFileStorage.queryExecuteStatus` and `batchQueryExecuteFailsStatus` call
`RequestFuture`, and both use the timed `get(timeout, unit)` overload — nothing
relies on the old untimed 1s-capped `get()` returning.
- Added method-level Javadoc on `get(timeout, unit)` documenting that
expiry throws `TimeoutException` (not silent `false`), and on untimed `get()`
stating it is for `Future` contract compliance only.
2. **Worker survivability (F2, F4)**
- Widened `executeResponse()` to catch `Exception` and never let response
publishing kill the sole disruptor consumer.
- Added an explicit note on writer reuse after non-`IOException`: the
current `HdfsWriter`/`CloudWriter` write path serializes before any stream
mutation, so unchecked failures do not leave a torn mid-file record; a blind
close/`fs.create` reopen would truncate the fixed `wal.txt` path and is
intentionally not done. Pre-existing mid-write `IOException` partial-record
risk is unchanged by the catch widening.
3. **Batch wait behavior (F6, F8)**
- `batchQueryExecuteFailsStatus` now uses a **shared deadline** across
the batch (`writDataTimeoutMilliseconds` from the start of the wait loop), so a
stuck worker cannot block `storeAll`/`deleteAll` for `N × timeout`.
- Per-entry `TimeoutException` is logged at **WARN without a stack
trace**; other unexpected failures remain at ERROR.
4. **Sync-path test naming / Mockito (F5 + second bucket)**
- Renamed `HdfsWriterFlushSyncPathTest` → `HdfsWriterFlushCallCountTest`
and rewrote the class Javadoc to state clearly: method-call-count parity only —
**not** disk-sync-count parity and **not** evidence that #12058 CV is resolved.
- Updated `HdfsWriterDurableFlushTest` cross-reference accordingly.
- Added an explicit `mockito-junit-jupiter` test dependency to
`imap-storage-file/pom.xml` (also inherited from the root POM’s test
dependencies).
### Framing (per @DanielLeens / @nzw921rx)
- PR keyword is now **Related to #12058** (not `Fixes`).
- Root-cause wording is a hypothesis supported by wall profiles pointing at
the sync path, not a confirmed same-machine isolated CV result.
- Correctness fixes (`RequestFuture` / `WALWorkHandler` / shared batch
deadline) stand on their own; #12058 can remain open for the investigation
process you outlined.
Happy to re-work any of the above if you still want a stronger writer-reset
path or further description tweaks. Thanks again for the careful review.
--
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]
