[GRPC] Handle ShardSearchFailure properly - #20641
Conversation
📝 WalkthroughWalkthroughThis PR upgrades the opensearch-protobufs dependency from 1.2.0 to 1.3.0 and refactors ShardSearchFailure serialization to use newly available protobuf types while maintaining backward compatibility through a deprecated legacy conversion method. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
|
❗ AI-powered Code-Diff-Analyzer found issues on commit 7e7fb12.
The table above displays the top 10 most important findings. Pull Requests Author(s): Please update your Pull Request according to the report above. Repository Maintainer(s): You can Thanks. |
|
❌ Gradle check result for 75ef5bc: 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? |
Signed-off-by: Karen X <karenxyr@gmail.com>
|
❌ Gradle check result for a631bb2: 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? |
Signed-off-by: Karen X <karenxyr@gmail.com>
|
❌ Gradle check result for 79c8f06: 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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/proto/response/exceptions/shardoperationfailedexception/ShardOperationFailedExceptionProtoUtils.java (1)
41-51:⚠️ Potential issue | 🟡 MinorAdd missing space in the unsupported-exception message.
The default message concatenates the class name and “cannot” without spacing, making logs harder to read.
🩹 Proposed fix
default -> throw new UnsupportedOperationException( - "Unsupported ShardOperationFailedException " + exception.getClass().getName() + "cannot be converted to proto." + "Unsupported ShardOperationFailedException " + exception.getClass().getName() + " cannot be converted to proto." );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/proto/response/exceptions/shardoperationfailedexception/ShardOperationFailedExceptionProtoUtils.java` around lines 41 - 51, The default branch in ShardOperationFailedExceptionProtoUtils' switch builds an error message by concatenating exception.getClass().getName() and "cannot" without a space; update the default UnsupportedOperationException message in the switch (the default -> ... branch) to insert a space before "cannot" so it reads "... " + exception.getClass().getName() + " cannot be converted to proto." ensuring clearer logs.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
CHANGELOG.mdgradle/libs.versions.tomlmodules/transport-grpc/licenses/protobufs-1.2.0.jar.sha1modules/transport-grpc/licenses/protobufs-1.3.0.jar.sha1modules/transport-grpc/spi/licenses/protobufs-1.2.0.jar.sha1modules/transport-grpc/spi/licenses/protobufs-1.3.0.jar.sha1modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/proto/response/exceptions/shardoperationfailedexception/ShardOperationFailedExceptionProtoUtils.javamodules/transport-grpc/src/main/java/org/opensearch/transport/grpc/proto/response/exceptions/shardoperationfailedexception/ShardSearchFailureProtoUtils.javamodules/transport-grpc/src/main/java/org/opensearch/transport/grpc/proto/response/search/ShardStatisticsProtoUtils.javamodules/transport-grpc/src/test/java/org/opensearch/transport/grpc/proto/response/exceptions/shardoperationfailedexception/ShardOperationFailedExceptionProtoUtilsTests.javamodules/transport-grpc/src/test/java/org/opensearch/transport/grpc/proto/response/exceptions/shardoperationfailedexception/ShardSearchFailureProtoUtilsTests.javamodules/transport-grpc/src/test/java/org/opensearch/transport/grpc/proto/response/search/ShardStatisticsProtoUtilsTests.java
💤 Files with no reviewable changes (2)
- modules/transport-grpc/licenses/protobufs-1.2.0.jar.sha1
- modules/transport-grpc/spi/licenses/protobufs-1.2.0.jar.sha1
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: karenyrx
Repo: opensearch-project/OpenSearch PR: 20335
File: modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/proto/request/search/SearchSourceBuilderProtoUtils.java:155-172
Timestamp: 2026-01-02T19:23:29.698Z
Learning: The gRPC search API in OpenSearch is marked as "experimental" in official documentation, so changes to proto schemas that remove previously unsupported fields (those throwing UnsupportedOperationException) are not considered breaking changes.
📚 Learning: 2026-01-02T19:23:16.689Z
Learnt from: karenyrx
Repo: opensearch-project/OpenSearch PR: 20335
File: modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/proto/request/search/SearchSourceBuilderProtoUtils.java:155-172
Timestamp: 2026-01-02T19:23:16.689Z
Learning: In OpenSearch's gRPC transport module, treat removals of previously-unsupported fields (those throwing UnsupportedOperationException) from experimental proto schemas as non-breaking changes. When reviewing changes to proto-related Java code in this module, document that such removals do not count as breaking API changes, and ensure tests reflect compatibility expectations accordingly.
Applied to files:
modules/transport-grpc/src/test/java/org/opensearch/transport/grpc/proto/response/exceptions/shardoperationfailedexception/ShardOperationFailedExceptionProtoUtilsTests.javamodules/transport-grpc/src/main/java/org/opensearch/transport/grpc/proto/response/search/ShardStatisticsProtoUtils.javamodules/transport-grpc/src/test/java/org/opensearch/transport/grpc/proto/response/exceptions/shardoperationfailedexception/ShardSearchFailureProtoUtilsTests.javamodules/transport-grpc/src/test/java/org/opensearch/transport/grpc/proto/response/search/ShardStatisticsProtoUtilsTests.javamodules/transport-grpc/src/main/java/org/opensearch/transport/grpc/proto/response/exceptions/shardoperationfailedexception/ShardSearchFailureProtoUtils.javamodules/transport-grpc/src/main/java/org/opensearch/transport/grpc/proto/response/exceptions/shardoperationfailedexception/ShardOperationFailedExceptionProtoUtils.java
📚 Learning: 2026-01-02T19:23:29.698Z
Learnt from: karenyrx
Repo: opensearch-project/OpenSearch PR: 20335
File: modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/proto/request/search/SearchSourceBuilderProtoUtils.java:155-172
Timestamp: 2026-01-02T19:23:29.698Z
Learning: In the transport-grpc module, suggest and aggregations protos were removed from SearchRequestBody in protobufs 1.0.0 because they haven't been vetted for accuracy in the API specification. The URL parameter suggest support (suggest_field, suggest_mode, suggest_size, suggest_text) is a minimized subset and not intended as a replacement for full Suggester functionality.
Applied to files:
modules/transport-grpc/src/test/java/org/opensearch/transport/grpc/proto/response/exceptions/shardoperationfailedexception/ShardOperationFailedExceptionProtoUtilsTests.javamodules/transport-grpc/src/main/java/org/opensearch/transport/grpc/proto/response/search/ShardStatisticsProtoUtils.javamodules/transport-grpc/src/main/java/org/opensearch/transport/grpc/proto/response/exceptions/shardoperationfailedexception/ShardSearchFailureProtoUtils.javamodules/transport-grpc/src/main/java/org/opensearch/transport/grpc/proto/response/exceptions/shardoperationfailedexception/ShardOperationFailedExceptionProtoUtils.java
📚 Learning: 2026-01-02T19:23:29.698Z
Learnt from: karenyrx
Repo: opensearch-project/OpenSearch PR: 20335
File: modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/proto/request/search/SearchSourceBuilderProtoUtils.java:155-172
Timestamp: 2026-01-02T19:23:29.698Z
Learning: The gRPC search API in OpenSearch is marked as "experimental" in official documentation, so changes to proto schemas that remove previously unsupported fields (those throwing UnsupportedOperationException) are not considered breaking changes.
Applied to files:
gradle/libs.versions.tomlCHANGELOG.md
🧬 Code graph analysis (5)
modules/transport-grpc/src/test/java/org/opensearch/transport/grpc/proto/response/exceptions/shardoperationfailedexception/ShardOperationFailedExceptionProtoUtilsTests.java (1)
modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/proto/response/exceptions/shardoperationfailedexception/ShardOperationFailedExceptionProtoUtils.java (1)
ShardOperationFailedExceptionProtoUtils(24-54)
modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/proto/response/search/ShardStatisticsProtoUtils.java (2)
modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/proto/response/exceptions/shardoperationfailedexception/ShardOperationFailedExceptionProtoUtils.java (1)
ShardOperationFailedExceptionProtoUtils(24-54)modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/proto/response/exceptions/shardoperationfailedexception/ShardSearchFailureProtoUtils.java (1)
ShardSearchFailureProtoUtils(21-69)
modules/transport-grpc/src/test/java/org/opensearch/transport/grpc/proto/response/exceptions/shardoperationfailedexception/ShardSearchFailureProtoUtilsTests.java (1)
modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/proto/response/exceptions/shardoperationfailedexception/ShardSearchFailureProtoUtils.java (1)
ShardSearchFailureProtoUtils(21-69)
modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/proto/response/exceptions/shardoperationfailedexception/ShardSearchFailureProtoUtils.java (1)
modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/proto/response/exceptions/opensearchexception/OpenSearchExceptionProtoUtils.java (1)
OpenSearchExceptionProtoUtils(47-251)
modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/proto/response/exceptions/shardoperationfailedexception/ShardOperationFailedExceptionProtoUtils.java (1)
modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/proto/response/exceptions/shardoperationfailedexception/ShardSearchFailureProtoUtils.java (1)
ShardSearchFailureProtoUtils(21-69)
⏰ 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). (1)
- GitHub Check: gradle-check
🔇 Additional comments (23)
CHANGELOG.md (1)
32-32: LGTM — changelog entry added correctly.gradle/libs.versions.toml (1)
27-27: Remove unused version entry or add missing library alias.Line 27 defines
opensearchprotobufs = "1.3.0"but no library in the [libraries] section references this version. Either remove the unused version entry or add the missing library alias that uses it (e.g.,opensearchprotobufs = { group = "...", name = "...", version.ref = "opensearchprotobufs" }).modules/transport-grpc/licenses/protobufs-1.3.0.jar.sha1 (1)
1-1: Both checksum files are already in sync; published jar verification requires external network access.The two checksum files (
modules/transport-grpc/licenses/protobufs-1.3.0.jar.sha1andmodules/transport-grpc/spi/licenses/protobufs-1.3.0.jar.sha1) both contain the identical checksuma141d00a9de80085436c648502d1b015fd89b9f6, confirming they are kept synchronized. However, verification that this checksum matches the publishedorg.opensearch:protobufs1.3.0 jar requires downloading from Maven Central, which requires external network access outside the sandbox environment.modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/proto/response/exceptions/shardoperationfailedexception/ShardSearchFailureProtoUtils.java (2)
34-45: Good: new ShardSearchFailure proto mapping is complete.Field population mirrors the XContent shape and captures shard/index/node/reason as expected.
47-67: Legacy conversion keeps back-compat and omits primary as intended.The deprecated path preserves older clients while aligning with search-failure semantics.
modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/proto/response/search/ShardStatisticsProtoUtils.java (1)
61-68: Nice dual-population of failures and failures_2 for ShardSearchFailure.This keeps legacy clients working while enabling the new proto type.
modules/transport-grpc/src/test/java/org/opensearch/transport/grpc/proto/response/exceptions/shardoperationfailedexception/ShardSearchFailureProtoUtilsTests.java (5)
21-41: Covers nodeId propagation and reason presence for new proto.
43-55: Good guard for missing nodeId in new proto conversion.
57-78: Legacy path validated, including default primary=false.
80-92: Legacy conversion without nodeId is well covered.
94-112: Nice parity check between legacy and new proto data.modules/transport-grpc/src/test/java/org/opensearch/transport/grpc/proto/response/exceptions/shardoperationfailedexception/ShardOperationFailedExceptionProtoUtilsTests.java (1)
37-48: Good coverage for legacy primary default behavior.modules/transport-grpc/src/test/java/org/opensearch/transport/grpc/proto/response/search/ShardStatisticsProtoUtilsTests.java (11)
22-29: Covers basic shard-stat fields.
31-39: Negative skipped handling is well tested.
41-49: No-failure scenario validates empty lists.
51-59: Null failures input handled and tested.
61-90: ShardSearchFailure populates both legacy and new fields.
92-125: Back-compat mapping between legacy and new failure protos is verified.
127-145: Mixed-type expectations for failures_2 are covered.
147-186: Multi-failure dual-field consistency is validated.
188-212: Old-client legacy read path is thoroughly tested.
214-239: New-client failures_2 read path coverage looks solid.
241-270: Gradual migration scenario is well exercised.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In
`@modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/proto/response/exceptions/shardoperationfailedexception/ShardOperationFailedExceptionProtoUtils.java`:
- Around line 41-51: The default branch in
ShardOperationFailedExceptionProtoUtils' switch builds an error message by
concatenating exception.getClass().getName() and "cannot" without a space;
update the default UnsupportedOperationException message in the switch (the
default -> ... branch) to insert a space before "cannot" so it reads "... " +
exception.getClass().getName() + " cannot be converted to proto." ensuring
clearer logs.
---
Duplicate comments:
In `@modules/transport-grpc/spi/licenses/protobufs-1.3.0.jar.sha1`:
- Line 1: The review comment on the checksum file
modules/transport-grpc/spi/licenses/protobufs-1.3.0.jar.sha1 (contents:
a141d00a9de80085436c648502d1b015fd89b9f6) is a duplicate; resolve by removing
the redundant reviewer note or marking the comment as resolved in the PR so no
code change to the checksum file is made; ensure only one authoritative comment
remains referencing that checksum string.
|
❌ Gradle check result for 79c8f06: 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? |
Signed-off-by: Karen X <karenxyr@gmail.com>
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #20641 +/- ##
============================================
- Coverage 73.32% 73.26% -0.07%
- Complexity 72064 72072 +8
============================================
Files 5781 5781
Lines 329395 329414 +19
Branches 47525 47531 +6
============================================
- Hits 241536 241340 -196
- Misses 68507 68791 +284
+ Partials 19352 19283 -69 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
* [GRPC] Handle ShardSearchFailure properly Signed-off-by: Karen X <karenxyr@gmail.com> * null check Signed-off-by: Karen X <karenxyr@gmail.com> --------- Signed-off-by: Karen X <karenxyr@gmail.com> Signed-off-by: Ankit Jain <jainankitk@apache.org>
* [GRPC] Handle ShardSearchFailure properly Signed-off-by: Karen X <karenxyr@gmail.com> * null check Signed-off-by: Karen X <karenxyr@gmail.com> --------- Signed-off-by: Karen X <karenxyr@gmail.com> Signed-off-by: Aparajita Pandey <aparajita31pandey@gmail.com>
* [GRPC] Handle ShardSearchFailure properly Signed-off-by: Karen X <karenxyr@gmail.com> * null check Signed-off-by: Karen X <karenxyr@gmail.com> --------- Signed-off-by: Karen X <karenxyr@gmail.com>
3 participants
Description
Fix the ShardSearchFailure proto utils after API spec fix in opensearch-project/opensearch-api-specification#1040
Changes to the Search shard failure response - it will include a new
failures2section, like:Test Plan
Search request:
response:
Related Issues
Resolves #[Issue number to be closed when this PR is merged]
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.