SEZ9 commented on PR #10961: URL: https://github.com/apache/seatunnel/pull/10961#issuecomment-5230720671
Thanks for cleaning up the history and pushing the new head — having a single focused commit with `e9f509651ca662d9798b10678b508a693c4b1151` makes this much easier to review properly. I'll do a fresh full re-review of that head from scratch, since it's a genuinely new revision. A few things I'll specifically be verifying, so you can double-check them on your side as well: 1. **`MysqlDialect.java` is fully untouched** — this was the standing blocker, so I'll confirm the diff no longer includes it. 2. **The extra JDBC connection during factory creation** — my earlier concern about detection opening a connection twice (once in `KingbaseDialectFactory`, once in `KingbaseCatalogFactory`) still stands; if that's addressed or consolidated in the new head, great, otherwise it remains an open ask. 3. **Silent Postgres fallback on detection failure** — detection errors should at least be logged clearly rather than silently falling back, so users can tell why the wrong mode was picked. 4. **The hard compile-time dependency on `com.kingbase8.jdbc.KbConnection`** — please confirm classes that may load without the Kingbase driver on the classpath don't fail with `NoClassDefFoundError`. 5. **The `KingbaseDialect.fieldIde` visibility change** — since it was previously a public mutable field, I want to make sure nothing downstream relied on mutating it. On the CI flakiness: if the failing checks vary irregularly across reruns and touch modules unrelated to this PR, they may well be known flaky tests. Please share the specific failing job names/links from the latest run — if they're clearly unrelated I can help retrigger, but if any failure touches the JDBC connector modules we'll need to dig in before merging. I'll post the full re-review results on the new head shortly. Thanks for your patience through the iterations! <!-- streview-comment:102 --> -- 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]
