SEZ9 commented on PR #10961: URL: https://github.com/apache/seatunnel/pull/10961#issuecomment-5225134029
Thanks for cleaning up the history and pushing a new head — squashing into one focused commit definitely makes this easier to review. Since this is genuinely new code on the branch now, I'll do a fresh full re-review of the latest head rather than a reply-only follow-up. Two quick things that would help speed that up: 1. Please post the exact new head SHA in this thread so we're both certain we're looking at the same revision (given the earlier confusion, I'd rather pin it explicitly). 2. Confirming that `MysqlDialect.java` shows zero diff against the base branch is the first thing I'll check — if the "Files changed" tab no longer lists it, that blocker is resolved. Beyond the MySQL isolation point, please keep in mind the other concerns from my earlier full review, which I'll be verifying against the new head: - **Double connection during factory creation**: the detection connection was opened once in `KingbaseDialectFactory` and again in `KingbaseCatalogFactory`. Ideally detect once, or at minimum make sure both connections are reliably closed. - **Silent Postgres fallback**: if `getCompatibleLevel()` detection fails, swallowing the exception and quietly falling back to the Postgres path can produce confusing downstream errors. A warning log (or a fail-fast, depending on your reasoning) would be better than a silent fallback. - **`KingbaseDialect.fieldIde` visibility**: it was a public mutable field before; changing it to final alters the public surface, so please double-check nothing else relies on mutating it. - **Hard compile-time dependency on `com.kingbase8.jdbc.KbConnection`**: this class can be loaded in environments without the Kingbase driver on the classpath, so the reference should be reflection-based or otherwise guarded to avoid `NoClassDefFoundError`. If those are addressed (or you can explain why some aren't applicable in the new structure), I think this is close. I'll take a proper look at the new head once you confirm the SHA. Thanks again for your patience through the back-and-forth! <!-- streview-comment:89 --> -- 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]
