SEZ9 commented on PR #11556:
URL: https://github.com/apache/seatunnel/pull/11556#issuecomment-5611785377
@DanielLeens thanks for verifying these against the source at `ba7c8bd743c8`
rather than just the diff — I agree with your triage on all three.
**Issue 3 (regex `table.include.list` from raw `TableId`)** — agreed on
elevating this to a blocker. A backfill for a table like `order$archive` is
filtered to zero rows, yet the split still reaches the stop LSN and dispatches
`END`, so the data loss is invisible. Concrete ask: escape the per-split value
in `PostgresSourceFetchTaskContext.createConnectorConfig()` (e.g.
`Pattern.quote()` on schema and table separately, or an equivalent
Debezium-safe form), and add a test with a table name containing a regex
metacharacter that asserts the backfill actually captures the concurrent DML.
**Issue 1 (`debezium.slot.name` bypass)** — agreed, and folding the
happy-path coverage ask into the same seam makes sense. Concrete ask: run the
`SLOT_NAME_PATTERN` check against the effective `slot.name` after the
`putAll(dbzProperties)` merge, the same way `include.schema.changes` is
re-applied to stay authoritative; then add tests for (a) a valid `slot.name`
accepted, (b) an invalid `slot.name` rejected, and (c) `debezium.slot.name`
carrying an invalid value being rejected rather than silently overriding. That
also restores the `getSlotNameForBackfillTask()` Javadoc guarantee, since both
the backfill slot name and the `pg_drop_replication_slot('...')` concatenation
derive from that effective value.
**Issue 2 (`closeEnumerator` unreachable for the documented FAQ behavior)**
— agreed. Since the `HybridSplitAssigner` constructors hardcode `false` for
`releasesEnumeratorResourcesOnCompletion`, the FAQ text describing the
enumerator dropping the streaming slot on `slot.drop.on.stop=true` for
exactly-once initial jobs doesn't match what runs. Either the docs should
describe the actual behavior or the code path should be made reachable — I'd
lean toward fixing the docs in this PR and leaving the behavior change for a
follow-up unless the author wants to take it on here.
Still open from my side, but not blockers: backfill slot persistence after a
reader crash (persistent `*_st_backfill_*` slots retaining WAL), the `TXID_KEY`
change in `PostgresUtils.currentLsn` not being called out in the PR description
or docs, and the FAQ showing bare `slot.drop.on.stop` rather than `debezium {
slot.drop.on.stop = true }`.
Summary of what's blocking from my perspective: Issue 3 fix + test, and
Issue 1 post-merge validation + the three tests above. Happy to re-review once
a new head is pushed.
<!-- streview-comment:933 -->
--
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]
