DanielLeens commented on PR #11639:
URL: https://github.com/apache/seatunnel/pull/11639#issuecomment-5459124124
Thanks @SEZ9, this is another careful pass and every one of the eight issues
has a concrete, checkable claim behind it - appreciated. Responding to each,
with where I agree, where I'd push back on severity, and what I'm actually
doing about it.
**Context first, since it changes what "blocking" means right now**: the
`Build` check on this exact head (`ef25cb1e3e6`) is currently red for a reason
unrelated to any of the eight issues below. My own re-review earlier today
(2026-08-28T01:50 UTC) found that `connector-cdc-db2` - a Debezium-based CDC
connector merged into `dev` after this branch was opened, pulled in by the
2026-08-27 rebase - was never migrated to declare its own
`debezium-api`/`debezium-embedded`, so it no longer compiles at all (confirmed
against the fork's own job logs, not a guess). That is the actual current
blocker and needs to land before any of the items below can be validated
against a green build, so I'm folding the doc/test fixes I'm accepting from
this review into the same follow-up commit as the db2 fix rather than pushing
several small commits.
**Issue 1 (versionless `provided` deps, atomic coupling to the parent pom) -
agree on the fix, pushing back on High.** The coupling is real, but I'd
characterize it as a discoverability gap, not a silent-failure risk:
`connector-cdc-base/pom.xml`, `connector-cdc/pom.xml`'s relocated
`dependencyManagement`, and the shade filters are all in the same commit set of
this same PR - there's no partial-apply path within the PR itself. A downstream
cherry-pick that took only the base module's diff would fail loudly at the very
next `mvn compile` (unresolved artifact version, since a versionless `provided`
dependency has nothing to fall back to) - not silently degrade. Still, a
pointer comment costs nothing and helps exactly the backport scenario you're
describing, so I'll add one next to the versionless declarations in
`connector-cdc-base/pom.xml` pointing at `connector-cdc/pom.xml`'s management
block.
**Issue 2 (no runtime fail-fast for missing Debezium classes) - agree, will
add.** The failure mode you describe is real, though it's only reachable
through a configuration the docs already call unsupported (mixing pre- and
post-change plugin jars in one plugins dir). Still, turning an opaque
`NoClassDefFoundError` into a message that names the missing dependency and
points at `incompatible-changes.md` is cheap and strictly better for whoever
hits it despite the docs. Will add a
`Class.forName("io.debezium.embedded.EmbeddedEngine")` probe at
`DebeziumAdapter` resolution.
**Issue 3 (patched-vs-stock class race, no runtime detection) - agree it's
not fully closed, but it's narrower than it reads.**
`stockDebeziumCoreOverridesAreExcludedInEveryCdcShadeConfiguration` (added in
the last couple of commits) now mechanically parses both shade-filter lists and
asserts they match the expected exclude set byte-for-byte, including the `$*`
nested-class patterns - that catches a regression in the filter *content* at
build time, for every connector in this repo. What it can't catch is a
regression in the filter *mechanism itself* (a `combine.children` typo
elsewhere, or a plugin-config inheritance change) - your runtime
`ProtectionDomain`/code-source assertion idea would close that. I'd rather
stage that, plus the bigger FQCN-relocation idea, as a dedicated follow-up
rather than grow this PR's diff further - it's a packaging-strategy change, not
a scope change. Will open a tracking issue referencing this thread so it
doesn't get lost.
**Issue 4 (test fragile to jar-classpath / working-directory) - agree,
straightforward, will fix.** `PatchedDebeziumClassesTest`'s `Files.isDirectory`
check on the `ProtectionDomain` code source does assume an exploded
`target/classes` layout, and the two pom reads do rely on the working
directory. Will add a jar-entry fallback for the class enumeration and anchor
the pom reads to an explicit basedir instead of CWD.
**Issue 5 (downstream Maven consumers lose the curated exclusions) - agree,
doc fix.** Correct that `kafka-log4j-appender`, the glassfish jersey exclusion,
and the `zstd-jni` M1 override now live in `connector-cdc`'s
`dependencyManagement`, which a third party depending on `connector-cdc-base`
as a plain library dependency does not inherit. Will add the exact
`<exclusions>` block to both `incompatible-changes.md` migration notes (en/zh)
so it can be copy-pasted rather than reconstructed.
**Issue 6 (wall-of-text docs, no version stated) - agree on structure.**
Will split the Impact bullet into sub-bullets (upgrade-as-a-set / new
dependency declarations / shade exclusions) and add a fenced XML snippet with
the literal exclude patterns, both docs. On the release version: this PR is
still pre-merge and blocked on the db2 fix above, so I'd rather not hardcode a
version number that may shift before it actually lands - will fill that in once
we're closer to a release cut rather than commit to a guess now.
**Issue 7 (zstd-jni pin, native-lib drift risk) - one factual correction,
then agree on the ask.** The `1.5.5-5` pin (with the Apple-M1 rationale
comment) wasn't deleted - it moved to `connector-cdc/pom.xml`'s
`dependencyManagement` alongside `debezium-api`/`debezium-embedded`, same
idiom, same file, lines 94-98 at this head. So today every connector still
resolves through exactly one place, which is what you're asking for. What's
genuinely missing is a build-time guard against a *future* per-connector
override drifting away from that single pin - agree that's worth having. Will
add a maven-enforcer `dependencyConvergence`-style rule scoped to the CDC
modules as a non-blocking follow-up.
**Issue 8 (PR description contradicts the docs/pom comment on whether the
jar still ships `io.debezium` classes) - confirmed and fixed.** You're right,
the description said "`connector-cdc-base-*.jar` no longer contains
`io.debezium` classes" while the pom comment and both `incompatible-changes.md`
entries correctly say five patched classes remain. I've updated the description
just now to say the jar no longer contains the *stock* Debezium runtime classes
while the five patched classes stay, matching the actual docs.
**Summary of what happens next**: the db2 fix (mechanical, matching the
pattern already applied to the other seven connectors) is the real blocker and
lands first; Issues 1 (comment), 4, 5, 6 and 8 are folded into that same
follow-up since they're all doc/test-robustness fixes with no runtime behavior
change; Issues 2, 3 and 7 are accepted non-blocking follow-ups I'll track
separately so they don't get lost in a growing diff. Thanks again for staying
on this - the shade-filter and patched-class discipline in this thread has
meaningfully improved over the last several rounds, and Issue 8 in particular
was a real accuracy bug in my own PR description that's now fixed.
--
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]
