You are viewing a plain text version of this content. The canonical link for it is here.

Posted to notifications@kyuubi.apache.org by "LuciferYang (via GitHub)" <gi...@apache.org> on 2026/08/30 16:51:20 UTC

[PR] [KYUUBI #7688][UTIL] Resolve ctorImpl classes through the builder's configured loader [kyuubi]

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: notifications-unsubscribe@kyuubi.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org