eskabetxe commented on PR #4516:
URL: https://github.com/apache/flink-cdc/pull/4516#issuecomment-5598266362
> Thanks for doing this — the one-minor-per-commit staging makes it
reviewable in a way a single 160-file diff would not have been, and the 2.7.4
stopping point with the Java 11 rationale matches what I measured independently.
>
> I built the same migration separately before finding this PR (same 1.9.8 →
2.7.4 target, same Java 11 reasoning), so rather than open a competing PR I
would rather hand over what I found. Four items, in severity order. The first
three I can reproduce; the fourth is a judgement call about scope.
>
> ## 1. The Java 11 guarantee does not survive a JDK 17 build machine
> This one undercuts the PR's own headline claim, which is why it is first.
>
> The root pom sets `target.java.version` to 11, but the `java-17-target`
profile auto-activates on `<jdk>[17,)</jdk>` and hard-codes the compiler plugin
directly:
>
> ```
> <profile>
> <id>java-17-target</id>
> <activation><jdk>[17,)</jdk></activation>
> <properties><java.version>17</java.version></properties>
> ... <source>17</source><target>17</target> ...
> </profile>
> ```
>
> So a release built on a JDK 17 toolchain emits class-file major 61
regardless of the declared property. I verified this is true on `master` today
and unchanged on this branch. It predates this PR — but this PR is the one
whose argument rests on "we stay on Java 11", so it seems worth pinning down
here or in a companion change.
>
> Fix that worked for me: have both profiles derive `<source>`/`<target>`
from `${source.java.version}`/`${target.java.version}` and carry only the
`--add-exports`/`--add-opens` arguments their JDK actually needs. Then the
declared property is authoritative, and `-Dtarget.java.version=17` still works
for anyone who wants it.
>
> I am happy to send that as a separate small PR so it does not enlarge this
one.
>
> ## 2. `specific-offset` on the legacy MySQL source cannot start on a
GTID-enabled server
> Reproducible, and it is a startup hang rather than a failure.
>
> Debezium 2.7 calls `client.setGtidSet("")` in the "No GTID stored in the
offset" branch — on this branch that is
`BinlogStreamingChangeEventSource.java:341` in the forked copy. Debezium 1.9.8
set filename and position and left the client's GTID state alone.
>
> In `com.zendesk:mysql-binlog-connector-java:0.29.2`, `setGtidSet("")` sets
`gtidEnabled = true` and, because the argument is the empty string
specifically, skips assigning `gtidSet` — leaving it null. `gtidEnabled` has
exactly two `putfield` sites (the constructor and that setter), so the client
cannot leave GTID mode once latched.
>
> Observed on an equivalent 2.7.4 branch, against `gtid_mode=ON`: a startup
`NullPointerException` from `DumpBinaryLogGtidCommand#toByteArray`, after which
the engine logs `"Can't start the connector, will retry later..."` every 10
seconds indefinitely — zero records, no checkpoint progress, and no fast
failure. Reproduced twice.
>
> The incremental source is unaffected. `earliest-offset` and `timestamp` on
the legacy source also survive; it is `specific-offset` that breaks.
>
> I fixed it by handing the legacy path a client that refuses an empty/blank
GTID set, which is where the state actually latches and needs no knowledge of
which branch Debezium took. I have a test against a real `gtid_mode=ON` server
that fails without the fix — happy to contribute it here.
>
> ## 3. A fatal binlog-thread error hangs the reader instead of failing the
job
> `MySqlErrorHandler` now passes `null` for Debezium's new fourth
`ErrorHandler` argument (`replacedErrorHandler`). I did the same thing
initially. The consequence is that a fatal error raised on the binlog-client
thread is recorded on the error handler but never reaches `BinlogSplitReader`,
which stays blocked in `ChangeEventQueue.poll`.
>
> `flink-cdc-base`'s `IncrementalSourceScanFetcher` has its own
`checkReadException`, but I do not see an equivalent on the MySQL binlog reader
path on this branch (`getProducerThrowable` has no call sites). In CI this
shows up as a job that never finishes rather than a red build, which is
unpleasant to debug — it is how a MySQL test class in my own branch appeared
"flaky" for a while.
>
> ## 4. Removed `debezium.*` passthrough keys are silently ignored (scope
judgement)
> Users can set arbitrary `debezium.*` properties, and Debezium's
`Configuration` does not reject unknown keys. After this bump, a job carrying
`debezium.database.server.name` keeps starting, but the key no longer does
anything — which changes the offset partition key and triggers a **full
re-snapshot with duplicate rows** rather than an error.
>
> I added a validator that fails fast at job submission for the keys removed
between 1.9.8 and 2.7.4, with the replacement named in the message. Two things
I got wrong first time and would flag:
>
> * `never`, `schema_only` and `schema_only_recovery` are all **still
valid** at 2.7.4 (the enum carries
> both spellings), so a 3.x-era list of removed values is wrong here. Only
Postgres's `exported`
> actually disappears between 1.9.8 and 2.7.4.
> * `database.history.instance.name` never existed; the real 1.9 key is
`database.history.name`.
>
> Whether that belongs in this PR or a follow-up is your call — it is a
behaviour change, not a bump.
>
> ## Things you caught that I did not, or did better
> * Your `PostgresOffsetUtils` fix also covers `transaction_id` and the
incremental-snapshot prefix
> keys; mine only handled the transaction-order counters. Yours is the
more complete allow-list.
> * The jsr305 demotion at 2.5 taking `javax.annotation` off the compile
classpath, including in
> `mongodb-cdc` which has no Debezium fork at all — I hit that as a
mystery build break and did not
> diagnose it as cleanly.
> * Staging by minor is simply better than my topical commits for bisecting
an upgrade like this.
>
> Happy to send the Java-target fix as its own PR, and to contribute the
GTID reproduction and test here if useful.
@jeff-jk thank you for the review, this is exactly the kind of handover that
makes a migration like this safer. I reproduced or traced each item against
this branch, notes per point below, in your order.
## 1. Java 11 guarantee vs. the `java-17-target` profile
Agreed, and confirmed on my side: the profile auto-activates on
`<jdk>[17,)</jdk>` and hard-codes `<source>17</source>/<target>17</target>`, so
the declared `target.java.version` is not authoritative on a JDK 17 toolchain.
It's on `master` unchanged and this branch doesn't touch those profiles, so
it's genuinely pre-existing, but you're right that it undercuts the specific
claim this PR leans on.
I'd rather keep this bump free of unrelated build-system changes, so
**please do send it as the separate PR you offered**, deriving both profiles'
`<source>`/`<target>` from `${source.java.version}`/`${target.java.version}` is
the right shape. Happy to review it.
## 2. `specific-offset` + GTID hang
This one I fully reproduce it, and it's the one I most want to fix here.
- The call is `client.setGtidSet("")` in the forked
`BinlogStreamingChangeEventSource`, verbatim from upstream 2.7.4, so inherited
rather than introduced, but present all the same.
- Binlog client resolves to
`com.zendesk:mysql-binlog-connector-java:0.29.2`, matching what you saw.
- Decompiling `BinaryLogClient#setGtidSet`: a `null` argument returns early,
but the empty string sets `gtidEnabled = true` and then the `arg.equals("")`
guard jumps **past** the `gtidSet` assignment — so `gtidEnabled` latches true
while `gtidSet` stays null, which is the NPE path through
`DumpBinaryLogGtidCommand#toByteArray` you described.
So: real, legacy-`DebeziumSourceFunction`-only, `specific-offset`-only on
`gtid_mode=ON`. **Yes please, contribute the reproduction and the test here**,
a client that rejects an empty/blank GTID set at the latch point is the right
place to fix it, since it's independent of which branch Debezium takes. I'll
fold it into the hop where the behaviour first regresses.
## 3. Fatal binlog-thread error → hang
I confirmed the mechanical facts ,`MySqlErrorHandler` passes `null` for the
fourth `replacedErrorHandler` argument, and `getProducerThrowable` has no call
sites on the MySQL path, but when I traced the base
`ErrorHandler.setProducerThrowable`, a **non-retriable** throwable still goes
to `queue.producerException(new ConnectException("…will be stopped"))`, which
does surface on the reader's `queue.poll()` (and `BinlogSplitReader` does have
a `checkReadException`). The thing `replacedErrorHandler = null` demonstrably
breaks is retry-count continuity across restarts (the base ctor only copies
`getRetries()` when it's non-null), which points more at a *retriable* error
looping than at a fatal error being swallowed, and that overlaps with the
10s-retry symptom in item 2.
I'd like to fix the right thing rather than just drop the `null`. Could you
share the exact exception + stacktrace from the class that appeared flaky for
you? If it's a genuinely swallowed fatal error I'm missing a path; if it's a
retriable error never exhausting its budget, the fix is about retry accounting
(or classifying that specific error). Either way I'll address it, just want to
confirm the mechanism first.
## 4. Removed `debezium.*` passthrough keys silently ignored
Agreed on the substance, `Configuration` doesn't reject unknown keys, so a
stale `debezium.database.server.name` keeps starting and silently changes the
offset partition key, i.e. a full re-snapshot rather than an error. And thank
you for the two corrections; they match what I see: `never` / `schema_only` /
`schema_only_recovery` are all still valid at 2.7.4 (only Postgres's `exported`
disappears in the 1.9.8→2.7.4 window), and the real 1.9 key is
`database.history.name`, not `database.history.instance.name`.
Since it's a behaviour change rather than part of the bump, I'd prefer to
keep it out of this PR and do it as a **follow-up** with the corrected
key/value list — a fail-fast validator at submission is the right call, just
not in the commit series that has to stay a pure version ladder. Happy to
co-author or review.
## On the rest
Appreciated the cross-check on `PostgresOffsetUtils` — the
incremental-snapshot prefix and `transaction_id` handling were exactly the
parts I was unsure I'd covered fully, so good to have that confirmed
independently. And glad the by-minor staging helped; that was the main bet of
this PR.
Following: I'll take **2** here (with your test), leave **1** and **4** to
the separate PRs you proposed, and circle back on **3** once I have the
stacktrace.
Thanks again for choosing to hand this over rather than fork the effort.
--
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]
