DanielLeens commented on PR #10306:
URL: https://github.com/apache/seatunnel/pull/10306#issuecomment-5231693443
## Follow-up review: verification of `da827df05` against the 2026-08-04
blockers
Scope note: my 2026-08-04 review was against `3da1fc962d`; the intermediate
hardening commits (`eb248ea13` JDBC savepoint boundary,
`7bab518ee`/`beebfe85f`/`08af4675b` outcome-marker rework) were covered in the
2026-08-07 round. This pass verifies the previously unreviewed `da827df05`
("Address DLQ review blockers") plus the current state of each blocker, by
reading the code at this exact head — not the PR comments.
### Blocker-by-blocker verification
**1. Default (unconfigured) JDBC path — FIXED.**
- `JdbcOutputFormat.flush()` fail-fast on SQLState `22`/`23` is now gated
behind the new `failFastOnRowLevelSqlState` flag, set from
`rowErrorCollector.isPresent()` via
`configureOutputFormatForRowErrorHandling()` — so the historical `max_retries`
loop is restored for every job that has not opted in.
`JdbcOutputFormatTest#testRowLevelSqlStateStillRetriesByDefault` pins the
default (2 executes with `maxRetries=1`) and
`#testRowLevelSqlStateFailsFastWhenEnabled` pins the opt-in path (1 execute).
- The non-collector `write()` branch is back to plain `tryOpen();
outputFormat.writeRecord(element);` — the `clearBatchSilently()` call that
could silently discard buffered rows under the MultiTableSink
continue-on-failure retry is gone from the default path.
- The now-moot `incompatible-changes.md` entry was correctly removed (both
locales), and `error-handling.md` accurately describes the gated behavior.
**2. `MultiTableState` serialVersionUID — PROVEN.**
`MultiTableSinkWriterTest#testMultiTableStateDeserializesPreFailedTablesCheckpoint`
deserializes a captured pre-PR state blob through `DefaultSerializer`. I
decoded the base64 stream: it contains exactly one field (`states
Ljava/util/Map;` — the pre-PR shape) and the embedded stream UID is
`0x53284AF97ECB9E44` = `5992121739651030596`, byte-identical to the declared
constant. So the constant genuinely is the old class's implicit `serialver`,
and old-checkpoint restore compatibility is demonstrated by an authentic
old-shape stream, not an assertion. (CI passing this test is the final stamp.)
**3. `EngineRowErrorCollector` retention — FIXED.**
The unbounded `recordedTerminalRows` map is gone entirely. After drain, a
row is only "remembered" by marking its **active probe**, and probes are
strictly scoped to in-flight writes: `MultiTableWriterRunnable.writeRow()`
pairs `beginCollectedRowErrorOutcomeProbe(row)` with either
`consumeCollectedRowErrorOutcome(row)` on success or
`clearCollectedRowErrorOutcomeProbe(row)` on both exception paths, so
`pendingTerminalOutcomeProbes` is bounded by concurrent writer threads. The
O(n²) scans are also fixed: `terminalOutcomes` and
`SinkFlowLifeCycle.pendingTerminalWriteRows` are now identity-keyed
`LinkedHashMap`s with O(1) consume/remove. I also walked the drain-vs-consume
race both ways (drain first → probe marked → worker skips double-count; consume
first → deferred WRITTEN deduped via `removePendingTerminalWriteRow`) — no
double counting in either interleaving.
**4. Threshold-counter docs vs implementation — RESOLVED via doc
correction.**
The docs no longer claim checkpoint-delta publishing; they now truthfully
state counters update shared engine state immediately, and the false "aborted
checkpoints do not advance committed thresholds" claim is gone. Note the
consequence that remains by design: with `mode != DISABLE`, every row performs
a distributed `IMap.compute` plus an `IMap.get` (the ratio-check argument in
`ErrorHandler.incrementTotalRecords`). That is an accepted, opt-in tradeoff now
that it's documented — but for high-throughput jobs it will be the dominant
cost of enabling the feature, so a local-cache/delta design remains worthwhile
follow-up work.
**5. `MultiTableSinkWriter.removeTableWriters()` — FIXED.**
Restored to `log.warn(..., "remove it anyway")` + unconditional removal: a
quarantined writer whose `close()` throws (typically because its connection is
the reason it was quarantined) no longer fails the whole job, so the
pre-existing continue-other-tables semantics are preserved.
### Remaining non-blocking items
-
`seatunnel-transforms-v2-e2e-part-2/src/test/resources/multi_table_merge.conf`
is still orphaned (no test references it — `TestTableMergeIT` runs
`table_merge_multi_table.conf`).
- The formatting-only files are still in the diff: three
`seatunnel-engine-ui` files (trailing newline),
`docs/zh/engines/zeta/realtime-observability.md` and
`rest-api-job-lifecycle.md` (trailing whitespace).
- `SinkFlowLifeCycle#getCollectedErrorCount/RoutedCount/DroppedCount` remain
unused.
- Error-handler counter entries in `engine_error-handler-counter-map` are
still never removed on job completion — unbounded (small-entry) growth on
long-lived clusters; fine as a follow-up issue rather than in this PR.
### Status
All five source-level blockers from my previous review are verified fixed at
`da827df05` — this matches @zhangshenghang's summary, and I confirmed each
against the actual code rather than the changelog. From my side there is no
outstanding source-level correctness blocker. The remaining gates are: (1) a
green `Build` on this exact head — it is currently queued and this head has not
completed CI yet, so test results are not in evidence — and (2) maintainer
approval per @SEZ9's triage. The minor cleanups above can ride along or land as
follow-ups at the maintainers' discretion.
*As before: conclusions are from source reading at `da827df05`; no local
build or E2E was run — CI remains authoritative for those.*
--
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]
