davidzollo commented on PR #10799: URL: https://github.com/apache/seatunnel/pull/10799#issuecomment-4528966540
Hi @vsantonastaso — really nice work tightening this PR down to a clean, minimal foundation. The `(staged)` in the title, the zero-diff against runtime classes, the preserved checkpoint format, and the `incompatible-changes.md` entry are all exactly the right shape for a foundation step. Compile is green across all 5 CDC modules on my end, and I'm comfortable with the overall direction. I have just three small items I'd like to see addressed before I dismiss my Changes-Requested. Each one should be quick: ### 1. SPI parameter semantics are inconsistent between `DebeziumAdapter` and `DebeziumAdapterFactory` `DebeziumAdapter.supports(...)` javadoc says the argument is a Debezium **fully-qualified class name** like `io.debezium.connector.mysql.MySqlConnector`, but `DebeziumAdapterFactory.getAdapter(String connectorType, ...)` names the same value `connectorType` and passes it straight through, which suggests a short name like `"mysql"`. A future implementer cannot tell which one is the contract. Could we pick one and align both sides? Two reasonable options: - **Option A** *(my preference)*: rename the factory parameter to `connectorClassName` and require FQCN, since that matches Debezium's own `connector.class` config convention. - **Option B**: keep the short-name semantics, update the `supports(...)` javadoc and example to `"mysql"` / `"postgres"` / etc. Either is fine — just pick one and make both files consistent. ### 2. The new SPI is currently dead code with no test, no registration, and no implementation `DebeziumAdapter` and `DebeziumAdapterFactory` are public API in `connector-cdc-base`, but: - `grep -rn "implements DebeziumAdapter" --include="*.java" seatunnel-connectors-v2/` returns zero results - `git ls-files | grep META-INF/services` shows no `DebeziumAdapter` registration - there is no unit test covering the factory - `DebeziumAdapterFactory.getAdapter(...)` will throw `IllegalStateException` for any call today I'm comfortable with the "target shape now, wire-up later" intent, but as published interfaces in `base` they will be hard to evolve later without source-incompat. Could we do **one** of: - **Option A** *(simplest)*: drop the two SPI files from this PR. The POM scope change + `incompatible-changes.md` is already a complete and self-contained foundation step; the SPI shape can land together with its first real implementation in the follow-up PR. - **Option B**: keep the SPI, but please add (a) a tiny `TestDebeziumAdapter` under `src/test/java` + a matching `src/test/resources/META-INF/services/...` entry, (b) a small unit test that exercises both the "found" and "not found" branches of `DebeziumAdapterFactory.getAdapter(...)`, and (c) an `@Experimental` (or similar) marker on the interface so we have room to change the shape without a public-API break. ### 3. `incompatible-changes.md` lists four connectors but the PR touched five Both the EN and ZH versions say: > Each CDC connector module (MySQL, PostgreSQL, Oracle, SQL Server) now explicitly declares... but this PR also updates `connector-cdc-mongodb/pom.xml` with the same `<debezium.version>` property + explicit `debezium-embedded` + `zstd-jni`. Anyone who maintains a third-party MongoDB CDC fork could miss the migration note. Could we add MongoDB to the list in both files (and, while we're there, the ZH file at line 65–66 could use a blank line between the previous bullet and the new one so the nested formatting renders cleanly)? --- That's it from me. Once these three are in, I'll dismiss the Changes-Requested and we can move on. Looking forward to picking up the real isolation story (base de-Debezium-ing + classloader isolation + cross-version PoC) in a follow-up — that one I think genuinely deserves a STIP discussion on dev@ before it starts. Thanks again for sticking with this — the final shape is much cleaner than where we started. -- 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]
