SEZ9 commented on PR #11659:
URL: https://github.com/apache/seatunnel/pull/11659#issuecomment-5190874001
@DanielLeens Thanks for the source-verified review — the PostgreSQL-CDC
findings were all correct and are fixed in head `aaaf0ec`:
**Issue 2 (blocker) — PostgreSQL-CDC option keys/format corrected** (Option
A, verified against `CatalogOptions.java`, `JdbcSourceOptions.java`,
`PostgresIncrementalSourceOptions.java`, and the shipped PostgreSQL-CDC doc):
- `table-name "schema.table"` → **`table-names` (plural, list-typed) with
fully qualified `database.schema.table` entries** (e.g.
`["postgres_cdc.inventory.orders"]`)
- `database-name` → **`database-names` (plural, list-typed)**
- the fabricated `publication.name` top-level option → replaced with the
real mechanism: **Debezium engine properties pass through the nested `debezium
{ }` map** (`debezium { publication.name = "my_pub" }`)
- the comparison-table row now contrasts `table-names` entry formats
(`database.table` vs `database.schema.table`), and the section ends by
deferring to `get_connector_info` for exact keys, consistent with the rest of
the file
- the items you verified as correct (`wal_level = logical`, per-job
`slot.name`, `decoding.plugin.name` default `pgoutput`) are unchanged
You're right that the measured-effect table covers only the routing/chain
tasks; the CDC section's key-name accuracy is now source-verified rather than
benchmark-verified, and the fair follow-up is a CDC-subset benchmark run once
this lands — noted for the umbrella tracking.
**Issue 1 (recommended) — routing diagnostics now carry the real section**:
`_validate_routing_pairs` messages use the actual `section.connector` location
(`transform.Sql: plugin_input "wrong_label" ...`, `already used by
transform.Sql`) instead of hard-coded `source.`/`sink.` prefixes, with two new
regression tests covering transform-originated dangling and duplicate labels.
Since the function lives in the routing-validation change, the same fix + tests
are applied on #11658 (where that code originates) and carried here.
On the `SkillRouter.match()` test suggestion: agreed it's the right lock-in
for the non-displacement claim, and noted it's a pre-existing gap for all 9
skills rather than this diff — I'd prefer to add a proper `test_skills.py`
covering all skills' trigger routing as a small follow-up rather than growing
this PR's scope. Happy to fold a minimal two-case version in here instead if
you'd rather see it now.
Full suite: 76 passed.
--
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]
