DanielLeens commented on PR #11971: URL: https://github.com/apache/seatunnel/pull/11971#issuecomment-5459099244
Thanks for the thorough follow-up review, @SEZ9 — this is a genuinely useful second pass on top of mine. Issue 1 (YashanDB `isSinglePhysicalTableQuery` closing the shared cached connection via try-with-resources on `getConnection(defaultUrl)`) is a real catch I missed in both of my rounds. I only checked that the *pattern* mirrored `AbstractJdbcCatalog.isSinglePhysicalTableQuery`/`OceanBaseMySqlCatalog`'s "don't close the shared connection" convention at a glance, not that `YashanDbCatalog` actually followed it — it doesn't, and closing the cached `defaultUrl` connection poisons the connection map for every later `tableExists`/`getTable` call on that catalog instance, not just this new code path. Agreed this is a High-severity blocker. On Issue 2 (OceanBase/YashanDB executing the full query a second time): this overlaps with my own Issue 2/recommended-fix from the previous round — I had it as a doubled-execution cost/side-effect concern and kept it non-blocking since it degrades to "slower, not wrong." Your framing (no `setMaxRows`/row cap on an arbitrary user query, so a `SELECT * FROM huge_table` can buffer client-side and OOM at planning time) is the sharper and more production-relevant version of the same root cause, and combined with Issue 1 landing in the same connection-handling code, I agree this should move to a blocker alongside Issue 1 rather than stay a fast-follow. Issues 3 and 4 (verified origin `TablePath` discarded in favor of the query-derived `TableIdentifier`'s path, and null database name for schema-only dialects swallowing into a silent `tableExists` failure) are both new findings I didn't trace — the TablePath-divergence one in particular is a correctness gap worth closing before merge, not just a follow-up, since it can attach the wrong table's PK/constraints under a same-named-table-in-another-schema collision. Issues 5/6 (incompatible-changes.md) match what I flagged as Medium/non-blocking last round — no disagreement there, just confirming alignment so @yzeng1618 doesn't need to reconcile two separate asks. Issue 7 is the same double-execution root cause as Issue 2 / my prior finding — agreed, one fix should close both. Issue 8 (DEBUG-level swallowing on every fallback path) is a fair catch given how much silent-failure surface this PR just added; I'd raise at least the loader-failure branch to `warn` with the table path included. @yzeng1618 — given Issue 1 and Issue 2 are now assessed as blocking (connection-pool poisoning plus an unbounded second execution on the same code path), please fold the fixes for those together with Issues 3/4/7/8 in the next commit. I'll hold off on a fresh full review until that lands, then re-review the whole diff against the new head rather than just the delta, since the connection-handling fix may touch the same lines as the double-execution fix. -- 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]
