DanielLeens commented on PR #11556:
URL: https://github.com/apache/seatunnel/pull/11556#issuecomment-5601184780
Thanks for the independent pass on `ba7c8bd743c8`, @SEZ9 — this is a
genuinely valuable second read and I don't want it to sit un-triaged against my
own `2026-09-09T00:46:19Z` review, so I pulled the actual source at this head
(via the contents API, not just re-reading the diff) to verify the three
findings with the highest correctness stakes before responding.
**Issue 3 (regex `table.include.list` from raw `TableId`) — confirmed, and I
agree this is the most serious item in your list.**
`PostgresSourceFetchTaskContext.createConnectorConfig()` at this head does
exactly `.with("table.include.list", tableId.schema() + "." + tableId.table())`
with no `Pattern.quote()` or equivalent escaping, and Debezium's
`RelationalTableFilters` does compile that value as a regex. A table like
`order$archive` would have its backfill silently filtered to zero rows, and per
the runtime chain you traced, `PostgresSnapshotFetchTask` would still reach the
stop LSN and dispatch `END` normally — a "successful" split that quietly drops
the exact concurrent-DML window this PR exists to capture. That's not a
hardening nit, it's a correctness gap in the PR's own core promise, and I'm
elevating it to a blocker alongside your Issue 1.
**Issue 1 (`debezium.slot.name` bypasses the new charset validation) —
confirmed.** I checked
`PostgresSourceConfigFactory.fromReadonlyConfig`/`create()` directly:
`checkArgument(SLOT_NAME_PATTERN...)` runs against `this.slotName`, then
`create()` does `props.setProperty("slot.name", slotName)` followed by `if
(dbzProperties != null) props.putAll(dbzProperties)` — and unlike
`include.schema.changes`, which is deliberately re-applied after that `putAll`
to stay authoritative, `slot.name` is not. So a `debezium.slot.name`
pass-through does silently override the validated value, and
`PostgresSourceConfig.getSlotNameForBackfillTask()`'s "guaranteed single-byte
ASCII" Javadoc claim no longer holds for that path. Given my own Issue 1 from
the last round (missing test coverage for the *happy*-path validation) is
really the same seam from a different angle, I'd fold both into one ask:
validate the effective post-merge `props.getProperty("slot.name")`, not just
the pre-merge field, and
add tests for both the accept/reject cases and the bypass case.
**Issue 2 (`closeEnumerator` unreachable for the documented FAQ behavior) —
confirmed.** `HybridSplitAssigner`'s two constructors both hardcode `false` for
`releasesEnumeratorResourcesOnCompletion` (with a comment explaining the
incremental phase's dependency, which is the right call for the flag's value) —
I checked both constructors directly. Since `IncrementalSource` only builds
`SnapshotOnlySplitAssigner` (the one that passes `true`) for
`StartupMode.SNAPSHOT_ONLY`, the only mode the FAQ text describes
(`exactly_once && INITIAL`) never takes the assigner that would call
`closeEnumerator` on a normal completion. This is a docs/Javadoc-vs-code
mismatch, not a data-safety bug, so Medium is the right severity — but it
should block merge alongside Issues 1/3 since it's describing a guarantee to
operators that doesn't exist.
I haven't independently re-verified Issues 4/5/6 line-by-line the way I did
for 1/2/3, but the evidence you've quoted is specific and consistent with what
I've already confirmed elsewhere in this file (the `DROP_SLOT_ON_STOP=false`
config is visible in the same
`createConnectorConfig`/`createReplicationConnectorConfig` methods I just
checked for Issue 3), so I have no reason to doubt them and I'm not going to
make you re-litigate them.
**Updated blocker list for this round, superseding the "test coverage only"
framing in my last review:**
1. Your Issue 3 — escape/quote the per-split `table.include.list`, or scope
the backfill dispatcher with an equality-based filter instead of the
regex-based include list. This is the one that can silently reintroduce the bug
the PR fixes.
2. Your Issue 1 — validate the effective (post-`dbzProperties`-merge)
`slot.name`, not just the pre-merge field; my own prior "no test coverage"
finding folds into this as the same fix's test.
3. Your Issue 2 — either fix the four docs pages + the `closeEnumerator`
Javadoc to describe what actually happens (nothing, on a normal `INITIAL`
shutdown), or wire a reachable enumerator-side drop for the hybrid path.
4. Carried over from my last review: get a clean CI run (sync `dev` first —
it already has the `DorisIT.testCustomSql` fix that's the likely cause of the
current `doris-connector-it` failure).
Issues 4/5/6 and my own carried-over Issue 3 (JDBC-connection-per-poll in
the test helper) stay non-blocking recommended fixes, same as before.
@davidzollo — sorry for the extra round of findings this late in the review;
Issue 3 in particular is worth prioritizing since it's the one with real
data-loss potential, and it's a small, self-contained fix (`Pattern.quote(...)`
at the two `table.include.list` call sites) that shouldn't need another design
discussion.
--
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]
