Rangsh commented on issue #12492:
URL: https://github.com/apache/seatunnel/issues/12492#issuecomment-5853977902
## Claim + proposed approach
Hi @SEZ9 @DanielLeens — I'll take this follow-up and open a separate PR
after we align on the approach below (keeping it out of #12081 as agreed).
I re-traced the current #12081 head (`7f69d7f69`) against LMAX Disruptor
`3.4.4` (the version this module depends on). Summary of what I think is
actually broken, and the fix I propose.
### What already works (from #12081)
- `WALWorkHandler` APPEND path uses sticky fail-close
(`appendBlockedAfterWriteFailure`) on `catch (Exception)`.
- Subsequent APPENDs get `done(false)` without touching the stream.
- `IMapFileStorage.isAppendPermanentlyBlocked()` → `FileMapStore` surfaces a
clear `IMapStorageException` ("permanently fail-closed… restart required").
- Production callers only use timed `RequestFuture.get(timeout, unit)` /
shared batch deadline — no unbounded hang on the hot path.
- Untimed `get()` already documents “prefer timed / Future-contract only” —
so I do **not** plan a Javadoc-only commit unless you still want wording tweaks.
### Residual gap (this issue)
`catch (Exception)` does **not** cover `Error`. If `writer.write(...)` (or
`executeResponse(...)`) throws an `Error`:
1. `executeResponse(...)` may never run → that request rides out only via
timed wait.
2. `appendBlockedAfterWriteFailure` is **not** set → callers do **not** get
the explicit permanently-blocked signal from `FileMapStore`.
3. The sole Disruptor consumer can still die. With
`handleEventsWithWorkerPool`, `WorkProcessor` catches `Throwable` and delegates
to the default `FatalExceptionHandler`, which **re-throws** (`RuntimeException`
wrap). That escape exits the processor loop and leaves the sole worker dead.
4. After that, further APPENDs sit unanswered until timeout / process
restart — the silent dead-worker mode called out here.
So this is not “callers hang forever” (timed waits still fire); it is
“pipeline dies without the fail-close / loud-failure contract that #12081
already established for `Exception`”.
### Proposed fix (PR scope)
Prefer aligning `Error` with the existing **fail-close + loud surface**
design over in-process worker restart / writer reopen (those remain unsafe for
the torn mid-file reasons already settled in #12081 F4).
1. **`WALWorkHandler` APPEND write boundary**
Widen the write-path catch from `Exception` to `Throwable`. On any
failure (including `Error`):
- set `appendBlockedAfterWriteFailure = true`
- always call `executeResponse(requestId, false)`
- **do not rethrow** on the APPEND path (same survivability shape as
today’s `Exception` handling) so the sole consumer stays alive in fail-closed
mode and subsequent APPENDs complete immediately with `false`
2. **`executeResponse` guard**
Widen its catch the same way (`Throwable`), so a late/missing future or
an `Error` from `done(...)` cannot take down the sole worker either (closes the
F2 residual you called out).
3. **Defense-in-depth on `WALDisruptor`**
Install a custom Disruptor `ExceptionHandler` via
`setDefaultExceptionHandler(...)` instead of relying on `FatalExceptionHandler`:
- log at ERROR with sequence / event context
- trip the same sticky fail-close on the `WALWorkHandler` instance when
possible
- **do not rethrow** (rethrow is what turns an already-handled escape
into a dead sole worker today)
- CLOSED / shutdown path can stay best-effort; the important contract is
steady-state APPEND
4. **No in-process writer reopen / worker restart in this PR**
Keep “restart the engine node” as the recovery story, matching the
existing fail-close Javadoc and `FileMapStore` messaging. Blind `fs.create`
reopen of fixed `wal.txt` stays out of scope.
5. **Regression test**
Extend `WALWorkHandlerSurvivabilityTest` (or add a sibling) to inject an
`Error` from `write(...)` and assert:
- `onEvent` returns normally (worker not killed by the handler boundary)
- failing future completes `false`
- sticky fail-close trips
- a subsequent APPEND completes `false` without a second `write(...)`
- optionally a small `WALDisruptor`-level test that an escaping throwable
does not leave the pipeline depending only on wait timeouts
### Explicit non-goals for the first PR
- Swallowing fatal JVM state without fail-close / logging (we still log; we
convert it into the existing permanent-block contract rather than a silent dead
consumer).
- Auto-restarting the Disruptor worker or reopening the WAL writer
in-process.
- Any change to #12081’s timed-wait / shared batch-deadline / Exception
fail-close behavior beyond Error coverage.
### Ask
Does this direction look right to you before I start coding?
In particular:
- OK to treat APPEND-path `Error` like `Exception` (fail-close + complete
future + keep consumer alive), rather than letting `FatalExceptionHandler` kill
the sole worker?
- OK to leave in-process restart as a later discussion / non-goal?
Happy to adjust based on your preference, then I’ll open the follow-up PR
against the post-#12081 baseline and link it here.
--
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]
