DanielLeens commented on PR #10779:
URL: https://github.com/apache/seatunnel/pull/10779#issuecomment-4518061616
Thanks for the update. I pulled the latest head
`c6ba752d51240a0180863c4f1a956920a78f4047` locally as `seatunnel-review-10779`,
reviewed the full diff against `apache/dev`, and retraced the real PostgreSQL
CDC schema-change runtime chain.
# What this PR fixes
- User pain: when PostgreSQL CDC sees DDL such as
add/drop/rename/type-change, users want SeaTunnel to forward those schema
changes downstream instead of handling DML only.
- Fix approach: add a PostgreSQL-specific DDL parser, a schema-change
resolver, a `schema-changes.enabled` option, and matching tests.
- In one sentence: this PR is trying to wire PostgreSQL CDC into SeaTunnel's
schema-evolution pipeline.
# Runtime path I checked
```text
User enables schema changes
-> PostgresSourceConfigFactory.create()
-> sets include.schema.changes in Debezium properties
Debezium deserialization path
-> PostgresIncrementalSource.createDebeziumDeserializationSchema()
-> SeaTunnelRowDebeziumDeserializeSchema
-> setSchemaChangeResolver(new PostgresSchemaChangeResolver(...))
-> this path is only hit if Debezium actually emits PostgreSQL DDL
schema-change records
Current repo baseline
-> connector-cdc/pom.xml
-> debezium.version = 1.9.8.Final
-> PostgresCDCIT.testPostgresCdcWithSchemaEvolution()
-> @Disabled("Debezium PostgreSQL 1.9 cannot emit DDL schema change
events to consumers.")
```
# Code review
## 1. Core logic
The parser / resolver work itself is reasonable. I can see the intended
chain from `include.schema.changes=true` in `PostgresSourceConfigFactory`,
through `PostgresIncrementalSource.createDebeziumDeserializationSchema()`, into
`PostgresSchemaChangeResolver`.
The blocker is one level above that code: on the current repository
baseline, the required PostgreSQL DDL schema-change events do not actually
reach the consumer side. The latest head itself acknowledges this by globally
disabling the schema-evolution E2E with `@Disabled("Debezium PostgreSQL 1.9
cannot emit DDL schema change events to consumers.")`, while the project still
pins `debezium.version = 1.9.8.Final`.
So the current implementation looks much closer to preparatory
infrastructure than to a feature that is actually available to users today.
## 2. Compatibility
Default behavior is still compatible because `schema-changes.enabled`
remains `false` by default. The risky part is the user-facing contract: once
someone enables the flag based on the current docs, they are being promised a
capability that the current dependency baseline does not actually deliver.
## 3. Performance / side effects
The parser/resolver overhead is fine. The real side effect is contract
drift: users can be told a feature exists even though the runtime path is still
unreachable.
## 4. Error handling / logging
Issue 1: the current repository baseline does not provide a reachable
PostgreSQL schema-change user path, but the docs and option contract already
advertise it as supported
- Location: `seatunnel-connectors-v2/connector-cdc/pom.xml:44`;
`connector-cdc-postgres/.../PostgresIncrementalSource.java:111-118`;
`seatunnel-e2e/.../PostgresCDCIT.java:746-753`;
`docs/en/connectors/source/PostgreSQL-CDC.md:104-119`
- Why this is a problem: the resolver path only matters if Debezium emits
PostgreSQL DDL schema-change records to consumers. The latest head explicitly
disables the only E2E for that path because Debezium PostgreSQL 1.9 cannot do
that, and the repo is still pinned to `1.9.8.Final`.
- Risk: users enable `schema-changes.enabled` and expect PostgreSQL CDC
schema evolution to work, but the actual event source is not there on the
current baseline.
- Best fix: either upgrade to a Debezium baseline that really exposes those
PostgreSQL DDL schema-change events and re-enable the E2E, or narrow the
user-facing scope/docs so this lands only as internal preparatory code instead
of an advertised feature.
- Severity: High
- Already raised by others: No
# Testing coverage and stability
The new unit tests around the parser and resolver are useful and
structurally stable. The problem is coverage of the real user path, not flaky
assertions: the key E2E is globally disabled, so CI does not currently prove
that the advertised capability works.
# Merge conclusion
### Conclusion: can merge after fixes
1. Blocking items
- Issue 1: do not ship this as a supported PostgreSQL CDC schema-evolution
feature until the Debezium baseline and the E2E validation path are both real.
2. Suggested follow-up items
- None beyond the blocker above; that one is the key merge gate.
Overall, I like the direction of the code, but the feature contract
currently gets ahead of what the pinned Debezium baseline can actually deliver.
Happy to re-review once that gap is closed.
--
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]
