JoegenUSTC commented on PR #11846: URL: https://github.com/apache/gravitino/pull/11846#issuecomment-4954640637
> I pushed a reference branch for the smaller/source-level fix here: > > https://github.com/yuqi1129/gravitino/tree/pr-11846-capability-boundary-fix > > The direction is: > > 1. Remove the enum `switch(scope)` from Hive/Glue capability implementations, so javac does not generate the synthetic `$1` switch-map class that can trigger the current `NoClassDefFoundError` after the catalog classloader is closed. > 2. Add a bounded capability execution path (`CapabilityHelpers.withCapability` / `CatalogWrapper.doWithCapabilityOps`) so capability logic runs while the wrapper classloader is active, and retry once if the cached wrapper was already closed. > 3. Migrate capability call sites to that entry point, instead of adding a full catalog lifecycle `ReadWriteLock` around every catalog operation. > > This keeps the fix focused on the source of the issue: do not let `Capability` escape its catalog/classloader boundary. Hi @yuqi1129, thank you for the thorough and well-evidenced review. Your analysis is correct on all points — I've updated the PR to follow your suggested direction. Changes made in response to your feedback: 1. Removed ReadWriteLock from all doWithXxxOps() methods. All read-lock/unlock boilerplate has been stripped from the 11 doWithXxxOps methods. Normal catalog operations no longer acquire any lock. 2. doWithCapabilityOps and close() now use synchronized (narrowly scoped). Only these two methods are mutually exclusive. close() uses catalog == null as the closed indicator — idempotent and simpler than a separate volatile boolean. 3. capabilities() now routes through doWithCapabilityOps. This ensures a concurrent close() produces a clean IllegalStateException rather than a cryptic NPE or NoClassDefFoundError, including for callers inside doWithTableOps/doWithViewOps lambdas. 4. Added retry-once in withCapability for stale wrappers. On IllegalStateException, if the wrapper is already closed and it's the first attempt, the wrapper is evicted and reloaded before retrying. 5. Replaced switch-on-enum with if/else in HiveCatalogCapability and GlueCatalogCapability. This directly eliminates the $1 synthetic switch-map class, removing the root-cause trigger entirely. The IsolatedClassLoader lifecycle is now irrelevant for these Capability implementations. 6. Added tests verifying that $1 is not generated, as you explicitly requested: TestHiveCatalogCapability#testNoSwitchOnEnumSyntheticClass and TestGlueCatalogCapability#testNoSwitchOnEnumSyntheticClass. 7. All getCapability() call sites fully migrated to withCapability(ident, mgr, fn) — 9 NormalizeDispatcher classes, OperationDispatcher.isManagedEntity(), CatalogManager.isManagedStorageCatalog(), and MetadataIdConverter.normalizeCaseSensitive(). There are zero remaining external callers of getCapability. The fix is now focused on the source of the issue: preventing Capability from escaping its catalog/classloader boundary, without introducing a full catalog-operation lifecycle lock. Thanks again for the reference branch and the detailed guidance — it made the right direction very clear. Would appreciate your review when you have a moment! -- 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]
