yuqi1129 commented on PR #10480: URL: https://github.com/apache/gravitino/pull/10480#issuecomment-4967326299
> @yuqi1129 While adding the docker-tagged ITs suggested in the review (r3535110319 and the testing issuecomment), the new `IcebergClassLoaderPoolIT` surfaced a failure in the existing `TestMultipleJDBCLoad`: when both run in the **same test JVM**, `TestMultipleJDBCLoad` fails with `Table '...iceberg_tables' doesn't exist`. > > I bisected it and — importantly — **reproduced it on `main` (without any `ClassLoaderPool`)** using a minimal probe: > > * `ProbeMainDropIT` (creates a MySQL-backed Iceberg catalog, then drops it) **alone** → passes > * `TestMultipleJDBCLoad` **alone** → passes > * the two **in the same JVM** → `TestMultipleJDBCLoad` fails identically (`iceberg_tables doesn't exist`) > > **Root cause (pre-existing on `main`, independent of this PR):** `IcebergCatalogWrapper.close()` unconditionally calls `AbandonedConnectionCleanupThread.uncheckedShutdown()` when a MySQL catalog is dropped. That is a **JVM-global** side effect, and it corrupts the shared MySQL/JDBC state of a _sibling_ MySQL-backed Iceberg catalog living in the same JVM. It was simply never triggered before, because no existing test **drops** a real MySQL-backed Iceberg catalog in a shared JVM (`TestMultipleJDBCLoad` only _creates_ catalogs). This PR's pool inherits the same global shutdown (relocated into `doFinalCleanup`), so the symptom is identical — the pool neither introduces nor worsens it. > > Since this is an existing test-isolation gap that is independent of the pool, I'd propose addressing it in a **precursor PR** — e.g. `forkEvery(1)` for the iceberg IT task so each IT class runs in its own JVM (which also mirrors production, where each server is a separate process) — and then rebasing this PR on top. Verified locally: with `forkEvery(1)`, the full combination (`IcebergClassLoaderPoolIT` + `TestMultipleJDBCLoad`) passes. > > Does that approach sound right to you, or would you prefer it bundled into this PR? Thanks for your hard work and for reporting this issue. I believe we can create a separate PR to solve the JDBC class clean-up problem first, then continue with this one. The current PR is already rather large, and an extra fix will hinder its being merged in a short time. -- 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]
