Rangsh commented on PR #12081:
URL: https://github.com/apache/seatunnel/pull/12081#issuecomment-5650309551
@DanielLeens @SEZ9 pushed `7c74bf982` addressing the latest review asks on
this PR (correctness track only; #12058 CV remains Related / separate).
### Issue 1 (blocking) — monitor failures must not abort already-persisted
checkpoint bookkeeping
- Guarded all `checkpointMonitorService.*` call sites in
`CheckpointCoordinator` via `notifyCheckpointMonitor(...)`: log at ERROR and
continue.
- Same helper pattern in `CheckpointManager` for `onPipelineRestored` /
`cleanupJob`.
- Added
`CheckpointCoordinatorTest.testCompletePendingCheckpointContinuesWhenMonitorThrows`:
monitor throws after a durable payload path → `completePendingCheckpoint` does
**not** throw, still decrements `pendingCounter` and removes the pending entry.
### Issue 2 — `deleteAll` failure keys
- `IMapFileStorage.deleteAll()` now does `requestMap.put(requestId, key)` to
match `storeAll()` (readable keys in failure / exception detail).
### Issue 3 — flush fail-close test stub
- `flushFailureFromHdfsWriterShouldFailCloseSubsequentAppend` now stubs the
one-arg `write(byte[])` overload that production calls (removed the inert
three-arg stub).
### SEZ9 — finishing the truncated “There was previously…” sentence
The full sentence on `6d7e999f0` was:
> There was previously no test that drove the failure through
`flush()`/`hsync` rather than `write()` throwing at the mock boundary. Added
`WALWorkHandlerSurvivabilityTest.flushFailureFromHdfsWriterShouldFailCloseSubsequentAppend`:
real `HdfsWriter` with a mocked `FSDataOutputStream` where
append/`write(byte[])` succeeds and only `hsync()` throws; asserts the same
fail-close contract plus `verify(out, times(1)).hsync()`.
No caveat path where flush/sync throws but the sticky flag is *not* set.
### #12058 boundary (unchanged)
This PR stays **Related only**. The isolated single-variable
`HdfsWriter.flush()` A/B on clean `dev` (no RequestFuture / WAL fail-close /
MapStore / Coordinator changes) is being run separately on #12058 and will be
reported there before any production CV conclusion.
Local verification:
```bash
./mvnw -pl
seatunnel-engine/seatunnel-engine-storage/imap-storage-plugins/imap-storage-file
\
-Dtest=WALWorkHandlerSurvivabilityTest,RequestFutureTest,HdfsWriterFlushCallCountTest,HdfsWriterDurableFlushTest,FileMapStoreTest
\
-DfailIfNoTests=false test
./mvnw -pl seatunnel-engine/seatunnel-engine-server \
-Dtest=CheckpointCoordinatorTest#testCompletePendingCheckpointContinuesWhenMonitorThrows
\
-DfailIfNoTests=false test
```
--
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]
