jeff-jk commented on PR #4516:
URL: https://github.com/apache/flink-cdc/pull/4516#issuecomment-5553999854
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:
```xml
<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.
--
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]
