LuciferYang opened a new pull request, #7689:
URL: https://github.com/apache/kyuubi/pull/7689
### Why are the changes needed?
Closes #7688.
`DynMethods.Builder.loader(ClassLoader)` configures the loader used for
name-based lookups, and `impl(String, ...)` honours it with
`Class.forName(className, true, loader)`. `ctorImpl(String, ...)` did not: it
built a fresh inner `DynConstructors.Builder`, whose own loader field defaults
to `Thread.currentThread().getContextClassLoader()`, and never forwarded the
configured one. So `.loader(x).ctorImpl("some.Class")` resolved the name
through the context loader rather than through `x`, and when the context loader
could not see the class the `ClassNotFoundException` was swallowed as a
candidate miss, leaving a generic cannot-find error with no hint that the
configured loader had been ignored.
Two things reviewers asked be stated plainly rather than left for someone to
discover.
First, `ctorImpl` has no caller anywhere in Kyuubi, only the new test. So
this is hygiene on a retained upstream API, not a fixed field bug. Iceberg's
answer to the same method was to delete it (in 1.7.0, apache/iceberg#10818);
dropping both `ctorImpl` overloads instead of fixing them is a reasonable
alternative, and a maintainer call rather than mine.
Second, the fix changes when the default loader is captured.
`DynMethods.Builder.loader` is snapshotted at builder construction, whereas the
old code snapshotted the context loader at `ctorImpl` call time. A caller that
never calls `.loader(...)` but mutates the thread context loader between
`DynMethods.builder(...)` and `.ctorImpl(...)` now gets the earlier snapshot.
That is already how `impl(String, ...)` behaves, so it makes the class
consistent, but it is a behaviour change. Relatedly, a chain ending in
`orNoop()` can flip: when the configured and context loaders disagree about a
class, whether the chain lands on the real `Ctor` or on the NOOP sentinel
reverses, and the two are not interchangeable because `NOOP.bind(receiver)`
returns a `BoundMethod` while `Ctor.bind` throws.
The generic `Cannot find method: <name>` message is unchanged by this patch.
`ctorImpl` still discards the `problems` map that `DynConstructors` assembles,
so a misconfigured loader still surfaces without a root cause. That is
pre-existing and left to a follow-up.
### How was this patch tested?
`testCtorImplUsesTheConfiguredLoader` in `DynMethodsTest`, in two halves
that fail in opposite directions without the fix.
The first sets the thread context loader to a `URLClassLoader` with a null
parent and no URLs, so it can reach no application class, and configures the
builder with the app loader. The lookup must succeed. Reverting the production
line makes `buildChecked()` throw `NoSuchMethodException: Cannot find method:
newInstance` before the assertion is reached.
The second flips them: context loader back to the app loader, builder
configured with the blinded loader. The lookup must now fail, which is what
pins the absence of a silent fallback. Reverting the fix makes the context
loader resolve the class, so `assertThrows` sees nothing thrown.
Both halves set the context loader explicitly rather than relying on the
ambient one, and the loader is restored in a `finally` that sits inside the
try-with-resources, so it is put back before the `URLClassLoader` is closed.
Ran on Zulu 17.0.18: `build/mvn -o test -pl kyuubi-util -am
-DwildcardSuites=none` gives 34/34 green with `DynMethodsTest` at 10 tests, and
`build/mvn -o spotless:check -pl kyuubi-util` is clean. The revert-to-red check
above was run in a throwaway worktree and reported `Tests run: 34, Failures: 0,
Errors: 1`.
### Was this patch assisted by generative AI tooling?
Assisted-by: Claude Opus 5
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
