DanielLeens commented on PR #11639:
URL: https://github.com/apache/seatunnel/pull/11639#issuecomment-5379306026
# What Problem Does This PR Solve?
- User pain point: `connector-cdc-base` ships the Debezium runtime
(`debezium-api`, `debezium-embedded`, and transitively `debezium-core` plus its
Kafka Connect tail) at compile scope, and because
`AbstractPluginDiscovery#filterPluginJar` force-loads
`connector-cdc-base-*.jar` into the class loader of every plugin whose name
contains "cdc", that one shared jar pins a single project-wide Debezium
version. No individual CDC connector can move independently.
- Fix approach: flip `debezium-api`/`debezium-embedded`/`zstd-jni` to
`provided` in `connector-cdc-base/pom.xml`, relocate the version + exclusion
`dependencyManagement` into the `connector-cdc` parent (interpolated per leaf
module against each connector's own `debezium.version`), have all seven
Debezium-based connectors declare their own runtime Debezium dependencies, add
a `maven-shade-plugin` filter dropping `debezium-core`'s stock copies of the
five classes `connector-cdc-base` patches, and add an inert `DebeziumAdapter`
SPI + guardrail tests per connector.
- One-sentence summary: a packaging-only refactor moving Debezium ownership
from the shared `connector-cdc-base` jar into each connector's own jar, with no
runtime behavior change for in-tree connectors, but with a documentation gap
for out-of-tree connectors that I flagged in my 2026-08-16 review and that is
still open today.
# 1. Code Change Review
## 1.1 Core Logic Analysis
**Status of this round.** Head is still
`afb0fada4b164062ef719f273caff9ad1197428d` (unchanged since 2026-08-03). No new
commit, no new comment/review from anyone (including @SEZ9) since my last check
on 2026-08-21. I did not merely re-read my own prior conclusions; I re-derived
the load-bearing facts against the current tree and against `dev`'s current
tip, and I checked one thing I had not specifically checked before:
- `git diff --stat` of this PR's own changes (merge-base `6bf786bee7cc` vs.
head) is still exactly the 31 files / +1719/-32 I traced in full in the
2026-08-16 review: the `connector-cdc-base` -> `provided` flip, the
`connector-cdc` parent `dependencyManagement` relocation, the `<filters
combine.children="append">` shade block, seven connectors' new Debezium
dependencies + `DebeziumAdapter` + `META-INF/services` + adapter test, and the
two docs. Zero `.java` files under existing production packages were touched by
this PR (only new files), consistent with every earlier round.
- I re-confirmed directly against the current head that
`connector-cdc-base/src/main/java/io/debezium/**` still ships exactly five
patched classes (`ChangeEventQueue`, `TableId`,
`HistorizedRelationalDatabaseConnectorConfig`, `HeartbeatFactory`,
`DefaultHeartbeatConnectionProvider`), and that `connector-cdc/pom.xml`'s shade
`<filters>` block still excludes only four of those five families from
`debezium-core` (`DefaultHeartbeatConnectionProvider` is still not excluded) —
Issues 1 and 3 below are unchanged.
- **New check this round:** `dev` gained a same-day commit after my last
review, `4ba289595` ("[Feature][shade] Refactor the seatunnel-shade module",
#9993, merged 2026-08-21 14:00, i.e. after my 00:29 check), which touches the
root `pom.xml` (115 lines) and, notably, `connector-cdc-base/pom.xml` (4
lines). Because this PR also edits `connector-cdc-base/pom.xml`'s dependency
block, I specifically checked whether #9993 conflicts with or invalidates the
shade-filter mechanism this PR depends on. It does neither: #9993's
`connector-cdc-base/pom.xml` hunk only swaps the module's `seatunnel-hikari`
dependency coordinate for the new `seatunnel-shade-hikari` artifact (a
pre-built shaded-jar rename, unrelated to Debezium), and its root `pom.xml`
hunk only adds new `seatunnel-shade-*` managed dependencies — it does not touch
the `maven-shade-plugin` `pluginManagement`/`<filters>`/`<execution>`
configuration my 2026-08-16 review traced (I grepped the full #9993 diff for
`maven-shade-plugi
n`; there are zero hits). So the plugin-inheritance chain that makes the
`debezium-core` exclusion filter reach all six inheriting connectors, and the
manual restatement required in `connector-cdc-opengauss`, are both still
exactly as I verified them.
- GitHub's own merge computation confirms no textual conflict has been
introduced by this drift: `mergeable: true`, `mergeable_state: blocked` (the
`REVIEW_REQUIRED` gate, not a conflict) — same as every prior round.
- `DebeziumAdapterFactory.getAdapter(...)` still has zero production call
sites; the SPI remains inert scaffolding, so the normal CDC read/snapshot path
is unaffected by this PR, as established previously.
I am not restating the full plugin-inheritance derivation, the `zstd-jni`
analysis, or the openGauss/Postgres coupling check here since nothing about
them has changed and they were derived in detail in my 2026-08-16 review (still
visible above on this PR) — I re-verified their conclusions still hold against
the current head and against current `dev`, rather than re-deriving them from
first principles a second time.
## 1.2 Compatibility Impact
Judgment: **Partially incompatible** (unchanged). No in-tree connector's
config, API, checkpoint, or serialization surface is touched. All seven
Debezium-based connectors still resolve `debezium.version=1.9.8.Final`, so the
class set on a running CDC job's class loader is identical before and after.
The breaking surface remains scoped to third-party/out-of-tree CDC connectors
that relied on `connector-cdc-base` to transitively supply Debezium — and the
migration note for that audience is still incomplete (Issue 2).
## 1.3 Performance / Side-Effect Analysis
Unchanged from prior rounds: no CPU/memory/GC change on the job path; the
Debezium runtime (~8-9 MB per connector including `zstd-jni`) is now duplicated
into each of the seven connector jars instead of shared via
`connector-cdc-base`, growing the distribution by an estimated 45-55 MB (still
not quantified in the docs — Issue 6).
## 1.4 Error Handling and Logging
No new error-handling paths in production code; nothing to flag.
# 2. Code Quality Assessment
## 2.1 Coding Standards
Unchanged: comments throughout are unusually thorough and explain *why*, not
just *what*. License headers present on all new files.
## 2.2 Test Coverage and Test Stability
Test stability rating: **Stable** (unchanged) — all eight new test classes
are deterministic (compiled-class/classpath/`ServiceLoader` inspection only, no
sleeps/network/fixed ports/shared mutable state).
Coverage gap unchanged: none of the guardrail tests inspect a
packaged/shaded jar (so a regression in the shade-filter merge — the one
genuinely fragile, novel mechanism here — would not be caught by CI), and every
module currently resolving the identical `1.9.8.Final` baseline means the tests
cannot distinguish "the per-module `debezium.version` override is honored" from
"every module inherited the same parent default." This is Issue 4, carried over.
## 2.3 Documentation Updates
`docs/en` and `docs/zh` `incompatible-changes.md` remain updated and
symmetric, but the specific content problems I raised on 2026-08-16 (Issues 1
and 2) are both still present verbatim at the same lines.
# 3. Architectural Soundness
## 3.1 Elegance of the Solution
Unchanged assessment: this is a precise, long-term fix for the real root
cause (a force-merged shared jar pinning one project-wide Debezium version),
not a workaround.
## 3.2 Maintainability
Unchanged: net improvement, with the same two hand-maintained (not
machine-checked) fragile points as before — the shade-filter merge/override
interaction, and the correspondence between the patched-class list and the two
exclusion lists (which has already drifted in letter, per Issue 3).
## 3.3 Extensibility
Unchanged: the `DebeziumAdapter` SPI is a reasonable seam for future
per-connector isolation, still not wired into production code.
## 3.4 Historical-Version Compatibility
Unchanged: no impact on checkpoint/savepoint, offset tracking, or serialized
state — confirmed again this round that zero `.java` files under existing
packages were modified.
# 4. Issue Summary
| # | Issue | Location | Severity | Status |
|---|---|---|---|---|
| 1 | Docs and a pom comment claim `connector-cdc-base` no longer contains
`io.debezium` classes; it still ships five patched ones |
`docs/en/introduction/concepts/incompatible-changes.md:150`;
`docs/zh/introduction/concepts/incompatible-changes.md:143`;
`connector-cdc-base/pom.xml:45-57` | Medium | Still open, re-verified this
round |
| 2 | Third-party migration instructions omit the mandatory
`io.debezium:debezium-core` shade exclusion (and `zstd-jni`), leaving
out-of-tree connectors exposed to a silent, order-dependent class-resolution
race against the patched `ChangeEventQueue` (which exists specifically "to
avoid the OOM in snapshot phase") |
`docs/en/introduction/concepts/incompatible-changes.md:150`;
`docs/zh/introduction/concepts/incompatible-changes.md:143` | High | Still
open, re-verified this round |
| 3 | `EXPECTED_PATCHED_CLASSES` lists five classes; both shade filters
exclude only four (`DefaultHeartbeatConnectionProvider` omitted from both,
unexplained); the test never checks itself against the pom filters |
`.../PatchedDebeziumClassesTest.java:50-59`; `connector-cdc/pom.xml:148-161`;
`connector-cdc-opengauss/pom.xml:121-133` | Medium | Still open, re-verified
this round |
| 4 | Guardrail tests assert nothing about the packaged/shaded jar and
cannot prove the per-module `debezium.version` override is honored over the
parent default | `.../PatchedDebeziumClassesTest.java`; every
`.../debezium/*DebeziumAdapterTest.java` | Medium | Still open |
| 5 | `zstd-jni` moved to each connector without a Debezium-ownership
rationale, multiplying a 5.6 MB artifact seven times |
`connector-cdc-base/pom.xml:70-73` + seven connector poms | Medium | Still open
|
| 6 | Distribution size growth (est. +45-55 MB) documented only
qualitatively | `docs/en/.../incompatible-changes.md:151`;
`docs/zh/.../incompatible-changes.md:144` | Low | Still open |
| 7 | Redundant `*:*` signature filter restatement justified by a comment
that hedges on a merge rule that is in fact deterministic |
`connector-cdc/pom.xml:162-173` | Low | Still open |
| 8 | PR is still a draft, `mergeStateStatus: BLOCKED` (review-required
gate), and is now 105 commits behind `dev` | PR metadata | Low | Still open; no
conflict introduced by the drift (verified against #9993 specifically this
round) |
# 5. Merge Recommendation
### Conclusion: Ready to merge after fixes
1. **Blockers — must be fixed**
- **Issue 2 (High)**: complete the third-party migration instructions in
`docs/en` and `docs/zh` with the mandatory `io.debezium:debezium-core` shade
exclusion (and `zstd-jni`), including the note that a connector declaring its
own shade `<execution>` must restate the filter. Without this, out-of-tree CDC
connectors are left with an undocumented, non-deterministic snapshot-phase OOM
risk.
- **Issue 1 (Medium, bundled with the above)**: correct the "no longer
contains `io.debezium` classes" claim, which sits in the same doc bullet and
directly undercuts the fix for Issue 2.
2. **Recommended fixes — non-blocking**
- Issue 3: explain or close the `DefaultHeartbeatConnectionProvider`
asymmetry; ideally make the guardrail test check the pom filters directly.
- Issue 4: add a packaging-level assertion against a built connector jar.
- Issue 5: leave `zstd-jni` compile-scoped in `connector-cdc-base`, or
document why per-connector Debezium ownership requires per-connector zstd
ownership.
- Issue 6: replace the qualitative size note with a measured delta.
- Issue 7: fix the comment to state the real Maven merge rule.
- Issue 8: mark ready for review and rebase onto current `dev` before
merge.
**On the other reviewer:** @SEZ9's 2026-08-04 COMMENTED review ("LGTM, can
merge") predates all of the above findings and was explicitly conditioned on
verifying the shade filters in `connector-cdc/pom.xml` and
`connector-cdc-opengauss/pom.xml`, which were "not visible in this diff
excerpt" at the time. I have since verified those filters directly against the
real artifacts (see 1.1) and found the gap in Issue 2 that his review did not
have visibility into. I don't read his approval as still standing against the
current, fully-traced picture, since the thing he flagged as unverified is
exactly where the remaining High-severity issue lives.
**Overall assessment.** Nothing has changed at the code level since my
2026-08-16 full review: same 31-file diff, same five patched classes, same
shade-filter mechanism, same two open doc issues. The one new thing worth
checking this round — a same-day `dev` commit (#9993) that also touches
`connector-cdc-base/pom.xml` and the root `pom.xml` — does not touch the
`maven-shade-plugin` configuration this PR's mechanism depends on, and GitHub
still reports no merge conflict. The mechanism itself remains correctly
implemented for every in-tree connector; the outstanding blocker is exclusively
in the third-party migration documentation, which is a two-paragraph fix, not a
rework.
--
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]
