JoegenUSTC commented on PR #11707: URL: https://github.com/apache/gravitino/pull/11707#issuecomment-4776576663
> > 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. @diqiu50 @yuqi1129 — Thanks again for the valuable feedback on both PRs. I wanted to share a new development and get your thoughts. Following @diqiu50's direction to address the root problem, I opened #11755 with a JDK proxy approach. During review, @yuqi1129 raised a concern — supported by a small standalone test — that the proxy's `withClassLoader` wrapping may not be sufficient for the `$1` case, since the synthetic class is resolved by the defining classloader rather than the TCCL. His test showed the `if-else` version handles this more reliably, even in edge cases where the classloader has already been closed. This suggests the classloader lifecycle fix (ensuring `IsolatedClassLoader` isn't closed while `Capability` instances are still in use) is a broader problem that deserves its own careful design, now tracked in #11726. Given this, I'd like to gently revisit whether this PR could be merged as a well-scoped, targeted fix for the immediate problem. I fully respect @diqiu50's preference to avoid landing workarounds, and I remain committed to the lifecycle root fix in #11726. That said, since the root fix will take more time to design and validate properly, would you both be open to landing this as a stable interim improvement while #11726 progresses? Happy to hear your thoughts — and thank you both for the continued engagement. -- 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]
