DanielLeens commented on PR #10779:
URL: https://github.com/apache/seatunnel/pull/10779#issuecomment-4525120828
Thanks for the update. I re-reviewed the latest head
`c6ba752d51240a0180863c4f1a956920a78f4047` locally against `apache/dev`, and I
retraced the PostgreSQL CDC schema-change runtime path again from the consumer
side. I did not run Maven locally in this pass; this is a source-level review
only.
# 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
only DML.
- Fix approach
This PR adds a PostgreSQL-specific parser / resolver chain,
`schema-changes.enabled`, tests, docs, and E2E resources.
- One sentence
The parser/resolver code direction is still good, but on the current
Debezium 1.9.8.Final baseline the real PostgreSQL DDL user path is still not
reachable, and the latest commit makes that even more explicit by disabling the
schema-evolution E2E.
# Runtime path I rechecked
```text
User enables schema changes
-> Postgres source config
-> include.schema.changes passed to Debezium
Debezium consumer path
-> if PostgreSQL DDL schema-change SourceRecord is emitted
-> SeaTunnelRowDebeziumDeserializeSchema
-> PostgresSchemaChangeResolver
-> PostgresAlterTableParser
-> AlterTable*Event
-> downstream schema-evolution sink can consume it
Current repository baseline
-> connector-cdc/pom.xml:44
-> debezium.version = 1.9.8.Final
-> PostgresCDCIT.java:746-747
-> schema-evolution E2E explicitly disabled because Debezium
PostgreSQL 1.9
cannot emit DDL schema change events to consumers
```
# Core review
Compared with Daniel's previous latest-head review on
`6619d4c437ed8746e01b9cbcd10ddb8579664fa3`, the new delta is only one line in
the E2E:
```java
@Disabled("Debezium PostgreSQL 1.9 cannot emit DDL schema change events to
consumers.")
```
Key findings:
1. The production parser / resolver chain itself is still wired into the
right place.
2. The new follow-up change does not fix the feature-readiness gap; it
confirms it.
3. The repo is still pinned to `debezium.version = 1.9.8.Final`.
4. Docs still advertise `schema-changes.enabled` as a supported user feature
even though the only end-to-end path is now explicitly disabled for the current
baseline.
# Findings
Issue 1: on the current Debezium 1.9.8.Final baseline, the PostgreSQL DDL
schema-change user path is still unreachable, while docs and option contract
still advertise it as supported
- Location: `seatunnel-connectors-v2/connector-cdc/pom.xml:44`
- Problem description:
`PostgresIncrementalSource.java:110-118` correctly installs
`PostgresSchemaChangeResolver` into the consumer-side chain. But the latest
head also adds `@Disabled("Debezium PostgreSQL 1.9 cannot emit DDL schema
change events to consumers.")` to the schema-evolution E2E in
`PostgresCDCIT.java:746-747`, while the repo remains pinned to
`debezium.version = 1.9.8.Final` and the docs still present
`schema-changes.enabled` as an available user-facing feature.
- Potential risk:
Users can enable a feature that is documented as supported, but the actual
PostgreSQL DDL event source is still unavailable on the current dependency
baseline.
- Best improvement suggestion:
Option A: upgrade to a Debezium baseline that really exposes PostgreSQL
DDL schema-change events to consumers, re-enable the E2E, and then ship this as
a supported feature.
Option B: if this round is only meant to land preparatory infrastructure,
then narrow the docs / user-facing contract accordingly instead of presenting
the feature as already available.
- Severity: High
- Already raised by others: No
# Compatibility
- Default behavior remains compatible because `schema-changes.enabled` stays
`false`.
- The risk is not default compatibility; it is the user-facing feature
contract once the flag is enabled.
# Tests / coverage
- The parser/resolver tests are still useful and structurally stable.
- The key gap is not flaky structure; it is that the schema-evolution E2E is
now explicitly disabled on the current baseline.
# CI status
- GitHub currently reports:
- `mergeable=MERGEABLE`
- `mergeStateStatus=BLOCKED`
- `Build=FAILURE`
- I checked the top-level check metadata: it points to fork run
`zhangshenghang/seatunnel` run `26275639691`, and that fork `Build` run is
indeed failing. I did not fully root-cause that fork build failure in this
pass, because issue 1 is already a source-level blocker independently of CI.
### Conclusion: can merge after fixes
1. Blocking items
- Issue 1: please do not ship this as a supported PostgreSQL CDC
schema-evolution feature on the current pinned Debezium 1.9.8.Final baseline.
2. Suggested non-blocking follow-up
- No extra non-blocking items from my side beyond the main blocker above.
Overall, I still like the code direction. But the latest head's new
`@Disabled` line confirms that feature readiness and user-facing contract are
still ahead of what the current dependency baseline can actually deliver.
--
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]
