davidzollo commented on PR #11556:
URL: https://github.com/apache/seatunnel/pull/11556#issuecomment-5579479463
Resolved the merge conflict against `dev` (one file,
`PostgresSourceFetchTaskContext.java` — two independent additive hunks: this
PR's legacy 3-arg constructor and `close()` replication-connection cleanup vs.
`dev`'s new `lastRelationSchemas`/`relationSchemaBaseline` fields and
schema-listener cleanup; kept both sides) and pushed `ba7c8bd743c`, which also
closes out the non-blocking follow-ups from your last review round on top of
the Issue 1 fix already on this branch:
- **Issue 8** — `HybridSplitAssigner.close()` now uses try/finally so a
failure closing the snapshot assigner can't skip closing the incremental
assigner.
- **Issues 2/6** — `SnapshotSplitAssigner` now tracks whether
`openEnumerator` has an outstanding, unclosed call (`dialectOpened`).
`open()`'s catch-block cleanup and `close()`'s completion-gated cleanup share
that guard, so `closeEnumerator` can't fire twice for one `openEnumerator` —
closing the "slot does not exist on the second call" masking risk.
`dialect.openEnumerator(sourceConfig)` moved inside `open()`'s try block so a
partial failure there is also covered by the catch-side cleanup. Updated the
three existing `close()` gating tests to call `open()` first (matching real
lifecycle usage now that `close()` only releases what `open()` actually
acquired) and added a dedicated regression test for the double-close fix.
- **Issue 3 (and 5, which it subsumes)** — `PostgresSourceConfigFactory` now
validates `slot.name` against `^[a-z0-9_]{1,63}$` at config-factory time,
failing fast at job submission. Since that guarantees the name is single-byte
ASCII, the existing char-count truncation in `PostgresSourceConfig` is now
provably byte-accurate rather than resting on an unenforced assumption —
documented on both call sites instead of touching the truncation logic itself.
I did not add a dedicated unit test for the new validation call path itself (no
existing test in this file exercises `fromReadonlyConfig`, and building a
minimal-but-correct `ReadonlyConfig` for it blind felt riskier than the
one-line guard clause it's protecting); flagging that gap rather than shipping
an unverified test.
- **Issue 7** — added the `max_replication_slots >= parallelism + 1` sizing
note to all four PostgreSQL/OpenGauss EN/ZH docs.
**Issue 4 not addressed** — the deterministic backfill-slot-name collision
after a reader crash. A correct fix needs a drop-if-exists-with-retry (or
per-attempt naming) change inside the path that builds Debezium's replication
connection for the backfill slot, which I don't think I should design and ship
without a real PostgreSQL failure-injection test bed to verify the
retry/backoff behavior against. Given it's rated Medium/non-blocking, I'd
rather flag it here than guess.
`./mvnw spotless:apply` run on `connector-cdc-base` and
`connector-cdc-postgres` before pushing, no further formatting changes
produced. Per the SeaTunnel local-verification rule I'm not running the
modules' tests locally — GitHub CI on this head is the verification source of
truth.
--
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]
