[Plugins] Relax jar hell check when extended plugins share transitive dependencies

[karenyrx]

Description

Currently, k-nn extends transport-grpc, and neural-search extends knn, but when neural-search tries to extend transport-grpc (which is already in knn) it gives jar hell when trying to build

Test Plan

Tested with opensearch-project/neural-search#1665

• Before: neural-search fails to build with following error:

| Caused by: java.lang.IllegalStateException: jar hell! extended plugins [opensearch-knn, transport-grpc] have duplicate codebases with each other: [file:/home/runner/work/neural-search/neural-search/build/testclusters/integTest-0/distro/3.4.0-ARCHIVE/modules/transport-grpc/error_prone_annotations-2.24.1.jar, file:/home/runner/work/neural-search/neural-search/build/testclusters/integTest-0/distro/3.4.0-ARCHIVE/modules/transport-grpc/protobufs-0.24.0.jar, file:/home/runner/work/neural-search/neural-search/build/testclusters/integTest-0/distro/3.4.0-ARCHIVE/modules/transport-grpc/grpc-protobuf-lite-1.75.0.jar, file:/home/runner/work/neural-search/neural-search/build/testclusters/integTest-0/distro/3.4.0-ARCHIVE/modules/transport-grpc/failureaccess-1.0.2.jar, file:/home/runner/work/neural-search/neural-search/build/testclusters/integTest-0/distro/3.4.0-ARCHIVE/modules/transport-grpc/grpc-stub-1.75.0.jar, file:/home/runner/work/neural-search/neural-search/build/testclusters/integTest-0/distro/3.4.0-ARCHIVE/modules/transport-grpc/perfmark-api-0.27.0.jar, file:/home/runner/work/neural-search/neural-search/build/testclusters/integTest-0/distro/3.4.0-ARCHIVE/modules/transport-grpc/guava-33.2.1-jre.jar, file:/home/runner/work/neural-search/neural-search/build/testclusters/integTest-0/distro/3.4.0-ARCHIVE/modules/transport-grpc/grpc-netty-shaded-1.75.0.jar, file:/home/runner/work/neural-search/neural-search/build/testclusters/integTest-0/distro/3.4.0-ARCHIVE/modules/transport-grpc/grpc-api-1.75.0.jar, file:/home/runner/work/neural-search/neural-search/build/testclusters/integTest-0/distro/3.4.0-ARCHIVE/modules/transport-grpc/grpc-core-1.75.0.jar, file:/home/runner/work/neural-search/neural-search/build/testclusters/integTest-0/distro/3.4.0-ARCHIVE/modules/transport-grpc/transport-grpc-spi-3.4.0-SNAPSHOT.jar, file:/home/runner/work/neural-search/neural-search/build/testclusters/integTest-0/distro/3.4.0-ARCHIVE/modules/transport-grpc/grpc-protobuf-1.75.0.jar, file:/home/runner/work/neural-search/neural-search/build/testclusters/integTest-0/distro/3.4.0-ARCHIVE/modules/transport-grpc/grpc-util-1.75.0.jar, file:/home/runner/work/neural-search/neural-search/build/testclusters/integTest-0/distro/3.4.0-ARCHIVE/modules/transport-grpc/grpc-services-1.75.0.jar, file:/home/runner/work/neural-search/neural-search/build/testclusters/integTest-0/distro/3.4.0-ARCHIVE/modules/transport-grpc/transport-grpc-3.4.0-SNAPSHOT.jar]

• After:
  Newly emitted log shows the the intersection of all shared ancestor dependencies between opensearch-knn and transport-grpc:

{"level":"INFO","timestamp":"2025-11-26T08:11:04,080","thread":"main","file":"PluginsService.java", "line":"734","message":"Plugin [opensearch-neural-search] extends multiple plugins/modules that share common dependencies: [file:/usr/share/opensearch-all/opensearch-v3.x/modules/transport-grpc/transport-grpc-spi-3.3.1.jar, file:/usr/share/opensearch-all/opensearch-v3.x/modules/transport-grpc/failureaccess-1.0.2.jar, file:/usr/share/opensearch-all/opensearch-v3.x/modules/transport-grpc/guava-33.2.1-jre.jar, file:/usr/share/opensearch-all/opensearch-v3.x/modules/transport-grpc/protobufs-0.24.0.jar, file:/usr/share/opensearch-all/opensearch-v3.x/modules/transport-grpc/grpc-api-1.75.0.jar, file:/usr/share/opensearch-all/opensearch-v3.x/modules/transport-grpc/transport-grpc-3.3.1.jar, file:/usr/share/opensearch-all/opensearch-v3.x/modules/transport-grpc/grpc-services-1.75.0.jar, file:/usr/share/opensearch-all/opensearch-v3.x/modules/transport-grpc/grpc-protobuf-1.75.0.jar, file:/usr/share/opensearch-all/opensearch-v3.x/modules/transport-grpc/grpc-netty-shaded-1.75.0.jar, file:/usr/share/opensearch-all/opensearch-v3.x/modules/transport-grpc/perfmark-api-0.27.0.jar, file:/usr/share/opensearch-all/opensearch-v3.x/modules/transport-grpc/grpc-core-1.75.0.jar, file:/usr/share/opensearch-all/opensearch-v3.x/modules/transport-grpc/grpc-util-1.75.0.jar, file:/usr/share/opensearch-all/opensearch-v3.x/modules/transport-grpc/grpc-protobuf-lite-1.75.0.jar, file:/usr/share/opensearch-all/opensearch-v3.x/modules/transport-grpc/error_prone_annotations-2.24.1.jar, file:/usr/share/opensearch-all/opensearch-v3.x/modules/transport-grpc/grpc-stub-1.75.0.jar]. This is expected when extended plugins share common ancestors."}

Hybrid queries are successfully discovered via transport-grpc-spi

{"level":"INFO","timestamp":"2025-11-26T08:11:04,496","thread":"main","file":"GrpcPlugin.java", "line":"101","message":"Discovered QueryBuilderProtoConverter extension: org.opensearch.knn.grpc.proto.request.search.query.KNNQueryBuilderProtoConverter (handles: KNN)"}
{"level":"INFO","timestamp":"2025-11-26T08:11:04,496","thread":"main","file":"GrpcPlugin.java", "line":"101","message":"Discovered QueryBuilderProtoConverter extension: org.opensearch.neuralsearch.grpc.proto.request.search.query.HybridQueryBuilderProtoConverter (handles: HYBRID)"}
{"level":"INFO","timestamp":"2025-11-26T08:11:04,496","thread":"main","file":"GrpcPlugin.java", "line":"108","message":"Successfully loaded 2 QueryBuilderProtoConverter extensions"}

Hybrid queries are successfully registered with transport-grpc:

{"level":"INFO","timestamp":"2025-11-26T08:11:07,760","thread":"main","file":"GrpcPlugin.java", "line":"318","message":"Processing external converter: org.opensearch.neuralsearch.grpc.proto.request.search.query.HybridQueryBuilderProtoConverter (handles: HYBRID)"}
{"level":"INFO","timestamp":"2025-11-26T08:11:07,761","thread":"main","file":"GrpcPlugin.java", "line":"326","message":"Injected registry into converter: org.opensearch.neuralsearch.grpc.proto.request.search.query.HybridQueryBuilderProtoConverter"}
{"level":"INFO","timestamp":"2025-11-26T08:11:07,761","thread":"main","file":"QueryBuilderProtoConverterSpiRegistry.java", "line":"121","message":"Registered query converter for HYBRID: org.opensearch.neuralsearch.grpc.proto.request.search.query.HybridQueryBuilderProtoConverter"}
{"level":"INFO","timestamp":"2025-11-26T08:11:07,761","thread":"main","file":"GrpcPlugin.java", "line":"331","message":"Successfully injected registry and registered all 2 external converters"}
{"level":"INFO","timestamp":"2025-11-26T08:11:07,761","thread":"main","file":"QueryBuilderProtoConverterSpiRegistry.java", "line":"85","message":"Set registry on 26 converters"}

Related Issues

Resolves #20100

Check List

• Functionality includes testing.
• API changes companion pull request created, if applicable.
• Public documentation issue/PR created, if applicable.

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

• Bug Fixes

  • Relaxed jar-hell validation for extended plugins that share transitive dependencies: deployments are no longer blocked for this case, and informational logs record plugin names and overlap details while other conflict checks remain.

• Tests

  • Updated tests to verify no exception is thrown and to validate transitive dependency collection instead of expecting a failure.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

[github-actions]

❌ 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?

[peterzhuamazon]

@coderabbitai review

[coderabbitai]

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[coderabbitai]

Walkthrough

Relaxed the jar-hell check: when extended plugins share overlapping classpath URLs the code now logs an informational message with the plugin name and overlapping URLs instead of throwing an IllegalStateException, allowing loading to continue; added CHANGELOG entry documenting the change.

Changes

Cohort / File(s)	Summary
Changelog entry CHANGELOG.md	Added an Unreleased 3.x note: "Relax jar hell check when extended plugins share transitive dependencies" with PR link.
Plugin jar-hell check server/src/main/java/org/opensearch/plugins/PluginsService.java	In checkBundleJarHell, replaced the thrown IllegalStateException for overlapping URLs among extended plugins with a logger.info that includes the plugin name and the intersection; control flow continues without throwing.
Tests updated server/src/test/java/org/opensearch/plugins/PluginsServiceTests.java	Updated testJarHellDuplicateCodebaseAcrossDeps to expect no exception and to assert that transitive dependency URLs include both the plugin's jar and the duplicate jar (verifies transitive dependency collection instead of erroring).

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

• Focus review on:
  • server/src/main/java/org/opensearch/plugins/PluginsService.java — correctness of the new logging, exact conditions where the exception was removed, and that downstream JarHell checks still run.
  • server/src/test/java/org/opensearch/plugins/PluginsServiceTests.java — test intent and assertions updated to reflect relaxed behavior.
  • CHANGELOG.md — entry formatting and PR link.

Poem

🐰 I hopped between jars with a gentle sigh,

Two plugins shared paths but we didn't cry.
A quiet log whisper, no crash in sight,
Now shared codebeasts sleep through the night. ✨

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%.	You can run @coderabbitai generate docstrings to improve docstring coverage.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly and concisely describes the main change: relaxing jar hell checks for extended plugins sharing transitive dependencies.
Description check	✅ Passed	The PR description comprehensively covers the problem, test plan, results, and includes all required checklist items with explicit completion status.
Linked Issues check	✅ Passed	The code changes fully address issue #20100 by replacing the jar-hell exception with informational logging when extended plugins share transitive dependencies, and the test validates the new behavior.
Out of Scope Changes check	✅ Passed	All changes (CHANGELOG update, PluginsService logic modification, test update) are directly scoped to the jar-hell check relaxation objective.

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[peterzhuamazon]

This PR raised before coderabbitai is online on the repo, so I manually trigger it here.

[coderabbitai]

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 correct

The new info log on Lines [731-735] for intersection between accumulated urls and pluginUrls correctly 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.urls intersect any extended plugin’s URLs (real duplicate codebase for the plugin itself).
• Still running JarHell.checkJarHell on:
  • 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 debug once the behavior is well validated to avoid noisy logs, but info is reasonable initially for visibility.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 468f944 and d4e646b.

📒 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 change

The 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.

[github-actions]

❌ Gradle check result for 08166c6: 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?

[github-actions]

❌ Gradle check result for 08166c6: 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?

[github-actions]

❌ Gradle check result for 08166c6: null

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]

Actionable comments posted: 0

🧹 Nitpick comments (1)

server/src/test/java/org/opensearch/plugins/PluginsServiceTests.java (1)

558-558: Prefer hamcrest matcher for consistency.

The file predominantly uses hamcrest matchers (e.g., line 668, line 961). Consider using assertThat(deps, notNullValue()) instead of assertNotNull(deps) to maintain consistency with the rest of the test file.

🔎 Suggested change

-        assertNotNull(deps);
+        assertThat(deps, notNullValue());

📜 Review details

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 08166c6 and 5ef379c.

📒 Files selected for processing (1)

• server/src/test/java/org/opensearch/plugins/PluginsServiceTests.java (1 hunks)

🧰 Additional context used

🧬 Code graph analysis (1)

server/src/test/java/org/opensearch/plugins/PluginsServiceTests.java (2)

server/src/main/java/org/opensearch/plugins/PluginsService.java (1)

• PluginsService (91-894)

libs/common/src/main/java/org/opensearch/common/bootstrap/JarHell.java (1)

• JarHell (77-323)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (20)

• GitHub Check: gradle-check
• GitHub Check: precommit (25, ubuntu-24.04-arm)
• GitHub Check: precommit (25, macos-15-intel)
• GitHub Check: precommit (21, windows-2025, true)
• GitHub Check: precommit (25, macos-15)
• GitHub Check: precommit (25, windows-latest)
• GitHub Check: precommit (21, macos-15-intel)
• GitHub Check: precommit (21, windows-latest)
• GitHub Check: precommit (21, ubuntu-24.04-arm)
• GitHub Check: precommit (25, ubuntu-latest)
• GitHub Check: precommit (21, ubuntu-latest)
• GitHub Check: precommit (21, macos-15)
• GitHub Check: assemble (21, ubuntu-latest)
• GitHub Check: assemble (25, ubuntu-latest)
• GitHub Check: assemble (25, ubuntu-24.04-arm)
• GitHub Check: assemble (21, ubuntu-24.04-arm)
• GitHub Check: assemble (25, windows-latest)
• GitHub Check: assemble (21, windows-latest)
• GitHub Check: Analyze (java)
• GitHub Check: detect-breaking-change

🔇 Additional comments (1)

server/src/test/java/org/opensearch/plugins/PluginsServiceTests.java (1)

554-559: Test correctly reflects the relaxed jar-hell behavior.

The updated test properly verifies that shared transitive dependencies (diamond dependency scenario) no longer throw an exception and that the transitive URL tracking still functions correctly. The assertions confirm both the plugin's own JAR and the shared dependency JAR are included in the transitive dependency set.

[github-actions]

❌ Gradle check result for 5ef379c: 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?

[github-actions]

❌ Gradle check result for 5ef379c: 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?

[github-actions]

❌ Gradle check result for 5ef379c: null

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?

[github-actions]

❕ Gradle check result for 5ef379c: UNSTABLE

Please review all flaky tests that succeeded after retry and create an issue if one does not already exist to track the flaky failure.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 73.27%. Comparing base (01b48c3) to head (5ef379c).
⚠️ Report is 4 commits behind head on main.

Additional details and impacted files

@@             Coverage Diff              @@
##               main   #20103      +/-   ##
============================================
+ Coverage     73.25%   73.27%   +0.02%     
- Complexity    71790    71792       +2     
============================================
  Files          5795     5795              
  Lines        328299   328300       +1     
  Branches      47279    47279              
============================================
+ Hits         240489   240556      +67     
+ Misses        68471    68421      -50     
+ Partials      19339    19323      -16     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.