[Feature][Connector-V2] Refactoring to have per-connector version management of Debezium#10799
[Feature][Connector-V2] Refactoring to have per-connector version management of Debezium#10799vsantonastaso wants to merge 1 commit intoapache:devfrom
Conversation
98d521a to
417b1b8
Compare
|
Hi @vsantonastaso, I rechecked the current PR head locally as This is a broad CDC infrastructure refactor, and the normal paths do hit the new adapter layer: The direction is reasonable for per-connector Debezium version management: the PR keeps connector-specific Debezium construction in connector modules and registers adapters through There are still two review notes I would like to keep visible:
Conclusion: can merge after fixesBlocking item:
Suggested non-blocking improvement:
|
|
A great work! |
417b1b8 to
0fc3c85
Compare
|
Hi @vsantonastaso, thanks for pushing this Debezium-version isolation work. The direction is valuable: moving Debezium API differences out of What This PR Fixes
Core Flow ReviewedFindingsIssue 1:
|
0fc3c85 to
818569e
Compare
What This PR Fixes
Local Review BasisI reviewed the full diff locally against Current Runtime ChainFindings
Conclusion: can merge after fixesBlocking items:
Suggested non-blocking improvement:
Thank you for pushing this refactor forward. The broad direction looks right; the current blocker is that one piece of the new abstraction is not actually safe on the real CDC path yet. |
5c1bda9 to
91065ec
Compare
|
Hi @vsantonastaso, I rechecked the latest head locally after the new commit. What changed after Daniel's previous review
Runtime chain I recheckedFindings
Merge conclusionConclusion: merge after fixesBlocking item:
Non-blocking note:
Thanks for pushing this refactor through. The current head is in much better shape than the version Daniel previously blocked. |
|
@davidzollo the build is green, all action jobs passed on my forked repo. Addressed also the feedback mentioned in the previous comments |
This comment was marked as outdated.
This comment was marked as outdated.
DanielLeens
left a comment
There was a problem hiding this comment.
Hi @vsantonastaso, thanks for pushing this refactor forward. I agree with the problem statement: SeaTunnel CDC needs a path toward per-connector Debezium version management. However, after rechecking the current head locally, I do not think the current implementation is safe to merge yet.
The primary blocking concern is architectural, not stylistic.
- The PR introduces adapter interfaces and ServiceLoader, but connector-cdc-base is still materially coupled to Debezium types and behavior. The base module still owns core runtime classes such as JdbcSourceEventDispatcher, and those classes still depend directly on Debezium APIs. That means the PR has not yet demonstrated the stronger property it is aiming for: that different connectors can evolve against different Debezium lines without the base layer becoming the compatibility boundary again.
- In other words, the current code moves some construction logic into connector modules, but it does not fully remove Debezium-version-sensitive responsibilities from the shared base module. From a review perspective, this leaves the main architectural risk unresolved.
- I would therefore treat the multi-version Debezium isolation is not yet actually proven problem as the first blocking item. Before merge, I would expect either a stronger design boundary that keeps connector-cdc-base version-neutral in the critical paths, or concrete proof that the remaining base-level Debezium dependencies do not break the intended upgrade model.
There are also two code-level issues on top of that architectural blocker:
- The new DebeziumSchemaHistory path is not wired into the real runtime flow yet. In all four JDBC CDC fetch contexts, configure() still calls super.registerDatabaseHistory(...), so the normal path still goes through JdbcSourceFetchTaskContext.registerDatabaseHistory(...) and directly into EmbeddedDatabaseHistory.registerHistory(...). The new connector-specific registerDatabaseHistory(...) overrides are therefore dead code on the current path.
- Even if that path is later wired in, the new schema-history bridge currently reserializes every TableChange as create(...), which would lose ALTER and DROP semantics during recovery.
Because of the above, my recommendation is: request changes.
What I would like to see before merge:
- A clearer and enforceable architectural boundary proving that connector-cdc-base is no longer the Debezium-version lock point for the targeted runtime paths.
- The schema-history path actually switched to the new abstraction on the real configure() -> registerDatabaseHistory() -> recover() flow.
- A fix for preserving CREATE / ALTER / DROP semantics in the schema-history bridge.
- At least one configure-level regression test that exercises the real fetch-context startup path, not only the factory/config helper classes.
I like the direction, but in its current form this still looks like an intermediate refactor rather than a completed isolation boundary.
@vsantonastaso Thank you very much for making such an important and meaningful major optimization. Could you take a look at this comment from @DanielLeens? I think this comment makes a lot of sense. What do you think? |
|
Hi @vsantonastaso, thanks for the continued work here. I pulled the latest head locally again and rechecked the real CDC runtime path, especially the schema-history boundary. What this PR is trying to solve
Runtime path I checked Blocking issues
Conclusion: do not merge yet
I appreciate the direction here, and I think the topic-naming / dispatcher cleanup is useful. The blocker is that the most version-sensitive runtime boundary is still not truly isolated yet. |
30e458f to
45878d9
Compare
|
I re-reviewed the latest head after the new commit and traced the MySQL CDC schema-history path locally. What this PR solves
Runtime pathReview findingsIssue 1: The schema-history path still depends on shared serializer and storage classes, so the version-isolation boundary is not fully moved into the connector layer
Issue 2: schemaHistory.start() is not paired with lifecycle cleanup in the fetch task
Merge conclusionConclusion: Merge after fixes Blocking items:
Non-blocking suggestions:
CI status:
|
45878d9 to
e7251f6
Compare
…agement of Debezium Signed-off-by: vsantonastaso <vsantonastaso.dev@gmail.com>
e7251f6 to
aa8cbfa
Compare
Purpose of this pull request
This PR introduces an abstraction layer for Debezium version management
across all CDC connectors, enabling independent Debezium version upgrades
per connector without forcing a monolithic upgrade across the entire CDC
module.
This PR implements an adapter pattern with Service Provider Interface (SPI)
that:
debezium.versionpropertyDebeziumAdapter,DebeziumEventDispatcher,DebeziumSchemaHistory)while MySQL stays on 1.9.8)
Implementation Details:
New Abstraction Interfaces (in
connector-cdc-base):DebeziumAdapter- SPI for version-specific implementationsDebeziumEventDispatcher<P>- Version-agnostic event dispatcherDebeziumSchemaHistory- Version-agnostic schema historyDebeziumTopicNaming<T>- Version-agnostic topic namingTableChangeInfo- Version-independent table schema representationDebeziumEventDispatcherConfig- Builder for dispatcher configurationPer-Connector Adapters (created for each database):
PostgresDebeziumAdapter,PostgresEventDispatcherAdapter,PostgresSchemaHistoryAdapterMySqlDebeziumAdapter,MySqlEventDispatcherAdapter,MySqlSchemaHistoryAdapterOracleDebeziumAdapter,OracleEventDispatcherAdapter,OracleSchemaHistoryAdapterSqlServerDebeziumAdapter,SqlServerEventDispatcherAdapter,SqlServerSchemaHistoryAdapteradapter needed)
ServiceLoader SPI Registration:
META-INF/services/org.apache .seatunnel.connectors.cdc.base.debezium.DebeziumAdapterDebeziumAdapterFactory.getAdapter("postgres")Refactored Components:
JdbcSourceEventDispatcher- Now uses generic partition types*SourceFetchTaskContextclasses - Load adapters and createcomponents via SPI
EmbeddedDatabaseHistorycalls - Now useDebeziumSchemaHistoryabstractionDoes this PR introduce any user-facing change?
How was this patch tested?
Check list
New License Guide
incompatible-changes.mdto describe the incompatibility caused by this PR.