yuqi1129 commented on PR #10480:
URL: https://github.com/apache/gravitino/pull/10480#issuecomment-4902186518
### Testing suggestion
A high-level note that spans several files, so leaving it here rather than
inline.
The unit/integration tests use empty dummy classloaders
(`CATALOG_LOAD_ISOLATED=false`), which validates the reference-counting logic
but **not** the two things this PR actually claims: real Metaspace savings, and
no cross-catalog interference when a classloader is genuinely *shared*. Those
are runtime-only properties. I'd add docker-tagged ITs
(`@Tag("gravitino-docker-test")`) with real provider jars covering:
**A. Memory / GC actually reclaimed**
- Hold a `WeakReference` to the `IsolatedClassLoader`, drop the catalog so
refCount → 0, `System.gc()`, and assert `ref.get() == null`. Leftover
ThreadLocals, lingering threads, or un-deregistered drivers will keep it alive
and fail here — exactly what the current tests can't catch.
- Run a create-loop under `-XX:MaxMetaspaceSize` with `-Xlog:class+unload` /
NMT and assert committed Metaspace stays flat with sharing on vs. O(N) with it
off.
**B. Shared classloader must not "cross-talk"**
- Two catalogs with the same key but different other config (e.g. two
`jdbc-mysql` on different DBs): DDL on A must not affect B.
- Iceberg's JDBC backend uses the `uri` property, **not** `jdbc-url`, so
it's not in `DEFAULT_ISOLATION_PROPERTY_KEYS` — two mysql-backed Iceberg
catalogs will share one classloader/DriverManager registry; verify no
contamination.
- Same vs. different Kerberos principal: do a real UGI login and confirm
sharing doesn't overwrite `loginUser`.
**C. Lifecycle timing (refCount boundaries)**
- A & B share a real MySQL classloader; drop A, then **issue a real query on
B** — assert the driver isn't deregistered and
`AbandonedConnectionCleanupThread` isn't shut down.
(`testClosingOneCatalogDoesNotAffectOthers` only re-loads, so it can't see
this.)
- `testConnection` now calls `wrapper.close()`: with a live catalog sharing
the key, `testConnection` must acquire+release without dropping refCount to 0
and breaking the live catalog; with no live catalog it builds+tears down a
classloader and deregisters the driver — verify a subsequent real `create`
re-registers cleanly.
- Concurrent create/drop of same-key catalogs, driving refCount 0 ↔ N with
GC, to shake out "cleaned up but still in use".
**D. Regressions from removed cleanup**
- iceberg-rest-server: the removed `useDifferentClassLoader()` override /
JDBC cleanup — verify a REST-server JDBC-backend wrapper rebuild still works
(the original comment warned the driver couldn't be reloaded after deregister).
- `sharing.enabled=false`: assert behavior matches pre-PR (per-catalog
classloader + per-catalog cleanup), as a baseline regression guard.
Highest-ROI to start with: **A** (WeakReference GC assert) and **C**
(real-driver drop / testConnection not hitting a live sharer) — those directly
exercise the cleanup paths that pure counting tests can't reach.
--
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]
