JoegenUSTC commented on PR #11707: URL: https://github.com/apache/gravitino/pull/11707#issuecomment-4766112879
> > Hi @diqiu50 @yuqi1129, thanks for the valuable feedback! > > Two updates: > > > > 1. **Null check**: Updated to use `Preconditions.checkArgument(scope != null, "scope cannot be null")` per the Gravitino coding convention, with a dedicated test added to cover the null scope case. > > 2. **Root fix**: Opened follow-up issue [[Improvement] Ensure all Capability method calls are made within the correct classloader context #11726](https://github.com/apache/gravitino/issues/11726) to track ensuring all `Capability` method calls are made within the correct classloader context — agreed this PR is a workaround and the broader fix deserves its own issue. > > > > CI is now passing. Could this PR be merged as-is, with the root fix tracked in #11726? > > I don’t think this workaround should be merged. We should fix the real problem instead. Thanks for the clear direction, @diqiu50! I've implemented the root fix in a new PR: https://github.com/apache/gravitino/pull/11755 The approach wraps the `Capability` delegate in a JDK dynamic proxy inside `CapabilityHelpers.getCapability()`. The proxy's `InvocationHandler` executes every method call inside `classLoader.withClassLoader()`, so the correct classloader context is always active regardless of the `Capability` implementation style — no per-call-site changes needed. This makes the `switch-on-enum → if-else` change here unnecessary. I'll close this PR once the root fix is accepted. Would appreciate your review on the new PR 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]
