Make plugin @allocates estimator classes linkable - #158779
Draft
jdconrad wants to merge 5 commits into
Draft
Conversation
Enabling allocation tracking on serverless QA broke every script calling ml's domainSplit with a NoClassDefFoundError for MlAllocationEstimators. The annotation resolves fine at allowlist-load time, using the plugin's own class loader. But the pre-check emits an INVOKESTATIC that names the estimator's class, and bytecode can only reference a class by name. At link time the generated script's loader resolves that name through javaClassNamesToClasses, whose parent is lang-painless, not the plugin. The estimator was never registered there, so the name resolved to nothing. Register it where it is resolved. That map exists for exactly this: classes the generated code must link to but scripts must not see. The from_class target on the same allowlist line is already registered the same way. This affected all five plugin estimator classes, in ml, sql, eql, mapper-version and mapper-unsigned-long. Built-in estimators were unaffected because they live in lang-painless, the parent loader. The def path was unaffected too: it unreflects the estimator into a MethodHandle, which holds a direct reference and never resolves a name. A unit test cannot reproduce the failure, since one flat classpath means the parent loader resolves everything, which is why this got through. The test asserts the registration instead, and that the class stays unnameable, untyped, unlisted and memberless.
Contributor
🔍 Preview links for changed docs⏳ Building and deploying preview... View progress This comment will be updated with preview links when the build is complete. |
Contributor
ℹ️ Important: Docs version tagging👋 Thanks for updating the docs! Just a friendly reminder that our docs are now cumulative. This means all 9.x versions are documented on the same page and published off of the main branch, instead of creating separate pages for each minor version. We use applies_to tags to mark version-specific features and changes. Expand for a quick overviewWhen to use applies_to tags:✅ At the page level to indicate which products/deployments the content applies to (mandatory) What NOT to do:❌ Don't remove or replace information that applies to an older version 🤔 Need help?
|
A unit test cannot reach this failure: tests run on one flat classpath, so the generated script's parent loader resolves every estimator and linkage always succeeds. That is why the ml breakage reached serverless QA. The example plugin is a real plugin with its own class loader, so it does reproduce it. Adds an estimator class there, an allocating method annotated to use it, and the system property that makes the pre-check emit the call. Verified by reverting the fix: the script then fails with no_class_def_found_error naming the estimator, which is the QA failure exactly. The estimator class is deliberately not allowlisted, matching how the x-pack modules ship theirs, and a second case checks scripts still cannot name it. This doubles as the missing worked example of @allocates for plugin authors, which the example plugin had nothing on.
The pre-flight compile gate derives a compile task from every changed test file, but plugins/examples is its own Gradle build, so :plugins:examples:...:compileYamlRestTestJava does not resolve and the gate fails. Any PR touching a test there hits this, and because the gate is shared, every flakiness re-run is skipped with it. Skip those paths. The root build cannot run those tests anyway; they are covered by the example-plugins periodic job.
A def call resolves its target at runtime and charges through a MethodHandle, which holds a direct class reference and never resolves a name, so it works with or without the linkage fix. The emitted call is the one that broke. Covering both routes means neither can regress on its own. Adds a static allowlisted method too, whose estimator takes only the arguments rather than the receiver first.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Enabling allocation tracking on serverless QA broke every script calling ml's
domainSplitwith aNoClassDefFoundErrorforMlAllocationEstimators.The annotation resolves correctly at allowlist-load time, because
resolveAllocationEstimatoruses the plugin's own class loader. The problem is downstream: the pre-check emits anINVOKESTATICthat names the estimator's class, and bytecode can only reference a class symbolically, by name. At link time the generated script's loader resolves that name throughjavaClassNamesToClasses, and its parent is lang-painless, not the plugin. The estimator class was never registered there, so the name resolved to nothing. The resolvedMethodsitting on thePainlessMethodis a compile-time object and is not consulted during linking.The fix registers the class where it is resolved. That map exists for precisely this case — its own comment describes it as "all the classes that need to be available to the custom classloader ... but not necessarily whitelisted individually" — and the
from_classtarget on the very same allowlist line is already registered the same way byaddImportedPainlessMethod. Registering grants linkage only; script-visible types come fromcanonicalClassNamesToClasses, which this does not touch.All five plugin estimator classes were affected: ml, sql, eql, mapper-version and mapper-unsigned-long. Built-in estimators were never affected because they live in lang-painless, the parent loader. The
defpath was also unaffected, since it unreflects the estimator into aMethodHandlethat holds a direct reference and never resolves a name — which is why the tested path worked and the emitted path did not.Worth knowing for review: a unit test cannot reproduce this failure. Tests run on one flat classpath, so the parent loader resolves everything and linkage always succeeds; that is why it got through. The test therefore asserts the registration itself, which does fail without the fix, plus that the class gains no script-visible surface — not nameable by short or qualified name, not resolvable as a type, absent from the
_contextAPI, and with noPainlessClassso no callable members.