DanielLeens commented on PR #10961: URL: https://github.com/apache/seatunnel/pull/10961#issuecomment-5229130627
Thanks @SEZ9 for following up. Just to keep the thread anchored: the PR head is still `a0146d1748f66eda6f39687176dd07c1744f764f` — the same `dev`-merge commit I pushed on July 29, which you reviewed on July 30, and which I gave a full from-scratch re-review on August 7 (see above). There's no new commit since then, so there isn't a fresh SHA to pin yet — the "squashing into one commit" / new-head language reads like it's referring to @PZh101's June 2 update (`e9f509651c...`), which was superseded by my `dev`-merge in July. I went back through the current code for each of the four points you listed, since some overlap with things I already dug into on August 7: 1. `MysqlDialect.java` — confirmed zero diff against `dev`. That's been true since the June 2 head and survived the July 29 merge; I re-checked it again in my Aug 7 pass. That blocker is still resolved. 2. Double connection during factory creation — still real. `KingbaseDialectFactory.detectCompatibleMode()` opens its own `SimpleJdbcConnectionProvider`, and `KingbaseCatalogFactory.detectCompatibleMode()` opens a second, independent one for the same job. This is the same duplication I flagged as Issue 3 in my Aug 7 review — the inconsistent WARN (dialect factory) vs. ERROR (catalog factory) log levels on the same "detection failed, fall back" condition come from these two separate copies. 3. Silent Postgres fallback — also still real and asymmetric: the dialect factory catches, logs at WARN, and returns `null` (falls back to native Kingbase); the catalog factory catches, logs at ERROR, and also returns `null`. Same underlying duplication as #2, not a separate root cause. 4. `fieldIde` visibility / `KbConnection` hard dependency — I don't think either is a blocker as it stands. The field did move from a public mutable `public String fieldIde = FieldIdeEnum.ORIGINAL.getValue();` on the pre-PR base to `private final String fieldIde` now, but I grepped the module for any external `.fieldIde` access on a `KingbaseDialect` instance and found none — every match is an unrelated `this.fieldIde` assignment inside other dialects' own constructors. The no-arg `KingbaseDialect()` constructor is also preserved, so existing `new KingbaseDialect()` call sites still compile. On the `KbConnection` reference: `kingbase8` is `<scope>provided</scope>` in `connector-jdbc/pom.xml`, exactly like `mysql-connector-java`, and `MysqlDialect`/`MySqlTypeConverter` already reference `com.mysql.cj.MysqlType` directly inside their own method bodies under that same provided-scope pattern — pre-existing, unrelated to this PR, and it hasn't caused classloading problems in prod uction. Since `KbConnection` is only referenced inside `detectCompatibleMode()`, which only runs after `acceptsURL()` has already matched a `jdbc:kingbase8:` URL (i.e. the Kingbase driver must already be on the classpath for the underlying connection to succeed at all), I don't see a realistic `NoClassDefFoundError` path for users who never touch Kingbase. The two items I still consider actual blockers on this same head are the ones from my Aug 7 review that neither of us has re-raised here, so I want to make sure they don't get lost: the catalog factory (`KingbaseCatalogFactory.detectCompatibleMode`) always live-probes and ignores an explicitly configured `compatible_mode`, unlike the dialect factory which correctly short-circuits on it; and the MySQL-compat branch of `KingbaseDialect.getUpsertStatement()` builds `new MysqlDialect()` with no arguments, silently dropping the outer dialect's configured `field_ide`. I re-checked both against the current head and they're still present. Since there's no new commit past my Aug 7 review, I'm not treating this as a fresh full re-review round here — just aligning our notes on the unchanged code. @PZh101, once you have a commit addressing Issue 1 (catalog explicit-mode short-circuit) and Issue 2 (field_ide passthrough) from my Aug 7 comment, ping us both and I'll do a clean re-review of the new head. -- 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]
