-
Notifications
You must be signed in to change notification settings - Fork 2.3k
[Plugins] Relax jar hell check when extended plugins share transitive dependencies #20103
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
…odule/plugin Signed-off-by: Karen X <[email protected]>
a2af18b to
d4e646b
Compare
|
❌ Gradle check result for d4e646b: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
WalkthroughThe pull request relaxes the jar hell check in OpenSearch's plugin loader to permit extended plugins sharing transitive dependencies. Instead of throwing an IllegalStateException when multiple extended plugins have overlapping classpaths, the code now logs an informational message with the plugin name and overlapping URLs, allowing plugin loading to continue. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
|
This PR raised before coderabbitai is online on the repo, so I manually trigger it here. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
server/src/main/java/org/opensearch/plugins/PluginsService.java (1)
728-735: Relaxed transitive-overlap check matches intended semantics; logging looks correctThe new info log on Lines [731-735] for
intersectionbetween accumulatedurlsandpluginUrlscorrectly turns what used to be a fatal “duplicate codebases among extended plugins” error into diagnostic output, while:
- Still throwing when the candidate plugin’s own
bundle.urlsintersect any extended plugin’s URLs (real duplicate codebase for the plugin itself).- Still running
JarHell.checkJarHellon:
- The growing union of extended-plugins’ URLs, and
- The union of those URLs plus the bundle’s own URLs, and
- The plugin+core union later in the method.
This cleanly solves the “multiple plugins extend the same ancestor/module” diamond case without weakening actual jar-hell protection.
If you expect many such configurations in production, you might consider downgrading this to
debugonce the behavior is well validated to avoid noisy logs, butinfois reasonable initially for visibility.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
CHANGELOG.md(1 hunks)server/src/main/java/org/opensearch/plugins/PluginsService.java(1 hunks)
🔇 Additional comments (1)
CHANGELOG.md (1)
36-36: Changelog entry accurately documents the behavioral changeThe new “Relax jar hell check…” bullet on Line [36] is clear, correctly scoped to extended plugins sharing transitive dependencies, and links to the right PR. No further edits needed.
…odule/plugin
Description
Currently,
k-nn-> extendstransport-grpcandneural-search-> extendsknn, but whenneural-searchtries to extendtransport-grpc(which is already inknn) it gives jar hell when trying to buildTest Plan
Tested with opensearch-project/neural-search#1665
Newly emitted log shows the the intersection of all shared ancestor dependencies between opensearch-knn and transport-grpc:
Hybrid queries are successfully discovered via transport-grpc-spi
Hybrid queries are successfully registered with transport-grpc:
Related Issues
Resolves #20100
Check List
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.