BulkRequest: Make sure that indices Set is properly initialized when being deserialized from TransportRequest stream #20132
Conversation
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
Contributor
WalkthroughModified BulkRequest deserialization to populate the Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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)
Comment |
…being deserialized from TransportRequest stream Signed-off-by: Nils Bandener <[email protected]>
nibix
force-pushed
the
bulk-get-indices
branch
from
2738fa2 to
14a3573
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
server/src/main/java/org/opensearch/action/bulk/BulkRequest.java (1)
106-116: Deserialization now correctly reconstructsindicessetPopulating
indicesfromrequestsafter deserialization aligns with how alladd/internalAddpaths already maintain this set and directly addresses the bug whereindiceswas empty after transport. This is functionally correct and side‑effect free (constructor starts with an emptyindicesset).If you want to slightly simplify and reduce coupling to
DocRequest, you could consider an equivalent loop, but this is purely optional:- requests.stream().map(DocRequest::index).forEach(indices::add); + for (DocWriteRequest<?> request : requests) { + indices.add(request.index()); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
server/src/main/java/org/opensearch/action/bulk/BulkRequest.java(2 hunks)
⏰ 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 (21, macos-15)
- GitHub Check: precommit (21, windows-2025, true)
- GitHub Check: precommit (25, macos-15-intel)
- GitHub Check: precommit (25, macos-15)
- GitHub Check: Analyze (java)
- GitHub Check: precommit (25, ubuntu-24.04-arm)
- GitHub Check: precommit (25, windows-latest)
- GitHub Check: precommit (21, ubuntu-latest)
- GitHub Check: precommit (21, ubuntu-24.04-arm)
- GitHub Check: precommit (21, windows-latest)
- GitHub Check: precommit (21, macos-15-intel)
- GitHub Check: precommit (25, ubuntu-latest)
- GitHub Check: assemble (25, ubuntu-24.04-arm)
- GitHub Check: assemble (21, windows-latest)
- GitHub Check: assemble (25, windows-latest)
- GitHub Check: assemble (25, ubuntu-latest)
- GitHub Check: assemble (21, ubuntu-24.04-arm)
- GitHub Check: assemble (21, ubuntu-latest)
- GitHub Check: detect-breaking-change
🔇 Additional comments (1)
server/src/main/java/org/opensearch/action/bulk/BulkRequest.java (1)
35-43: Import ofDocRequestis appropriate and scopedThe new
DocRequestimport is only used for the method reference in the deserialization constructor and keeps the file’s dependency surface aligned with existing request types. No issues here.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
server/src/main/java/org/opensearch/action/bulk/BulkRequest.java (1)
115-115: LGTM! Correctly populates the indices set after deserialization.The logic mirrors the behavior of the
add()methods and ensures that theindicesset is properly initialized when deserializing aBulkRequest.Optional: Consider a traditional for-loop for improved readability:
- requests.stream().map(DocRequest::index).forEach(indices::add); + for (DocWriteRequest<?> request : requests) { + indices.add(request.index()); + }server/src/test/java/org/opensearch/action/bulk/TransportBulkActionTests.java (1)
387-397: LGTM! Test validates the deserialization fix.The test correctly verifies that the
indicesset is populated after serialization and deserialization of aBulkRequest.Optional: Consider enhancing test coverage by including multiple requests with different indices:
public void testSerializationDeserialization() throws Exception { - BulkRequest bulkRequest = new BulkRequest().add(new IndexRequest("index").id("id").source(Collections.emptyMap())); + BulkRequest bulkRequest = new BulkRequest() + .add(new IndexRequest("index1").id("id1").source(Collections.emptyMap())) + .add(new IndexRequest("index2").id("id2").source(Collections.emptyMap())) + .add(new DeleteRequest("index1", "id3")); MockBigArrays mockBigArrays = new MockBigArrays(new MockPageCacheRecycler(Settings.EMPTY), new NoneCircuitBreakerService()); try (ReleasableBytesStreamOutput out = new ReleasableBytesStreamOutput(mockBigArrays)) { bulkRequest.writeTo(out); BulkRequest deserializedRequest = new BulkRequest(out.bytes().streamInput()); - assertEquals(Set.of("index"), deserializedRequest.getIndices()); + assertEquals(Set.of("index1", "index2"), deserializedRequest.getIndices()); } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
CHANGELOG.md(1 hunks)server/src/main/java/org/opensearch/action/bulk/BulkRequest.java(2 hunks)server/src/test/java/org/opensearch/action/bulk/TransportBulkActionTests.java(3 hunks)
✅ Files skipped from review due to trivial changes (1)
- CHANGELOG.md
⏰ 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, macos-15)
- GitHub Check: assemble (25, ubuntu-24.04-arm)
- GitHub Check: precommit (21, macos-15)
- GitHub Check: precommit (21, windows-2025, true)
- GitHub Check: precommit (25, ubuntu-24.04-arm)
- GitHub Check: precommit (25, windows-latest)
- GitHub Check: precommit (21, macos-15-intel)
- GitHub Check: precommit (21, ubuntu-latest)
- GitHub Check: precommit (25, macos-15-intel)
- GitHub Check: precommit (21, ubuntu-24.04-arm)
- GitHub Check: assemble (21, ubuntu-latest)
- GitHub Check: precommit (25, ubuntu-latest)
- GitHub Check: assemble (25, ubuntu-latest)
- GitHub Check: assemble (21, ubuntu-24.04-arm)
- GitHub Check: assemble (21, windows-latest)
- GitHub Check: assemble (25, windows-latest)
- GitHub Check: precommit (21, windows-latest)
- GitHub Check: Analyze (java)
- GitHub Check: detect-breaking-change
server/src/main/java/org/opensearch/action/bulk/BulkRequest.java
Outdated
Show resolved
Hide resolved
Signed-off-by: Nils Bandener <[email protected]>
Contributor
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #20132 +/- ##
============================================
- Coverage 73.33% 73.22% -0.12%
+ Complexity 71679 71630 -49
============================================
Files 5790 5786 -4
Lines 327549 327755 +206
Branches 47181 47206 +25
============================================
- Hits 240217 240003 -214
- Misses 68080 68470 +390
- Partials 19252 19282 +30 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
cwperks
approved these changes
Dec 1, 2025
cwperks
reviewed
Dec 1, 2025
server/src/test/java/org/opensearch/action/bulk/TransportBulkActionTests.java
Outdated
Show resolved
Hide resolved
Signed-off-by: Nils Bandener <[email protected]>
Contributor
rgsriram
pushed a commit
to rgsriram/OpenSearch
that referenced
this pull request
Dec 5, 2025
…being deserialized from TransportRequest stream (opensearch-project#20132) * BulkRequest: Make sure that indices Set is properly initialized when being deserialized from TransportRequest stream Signed-off-by: Nils Bandener <[email protected]> * fix Signed-off-by: Nils Bandener <[email protected]> * simplified BytesStreamOutput construction Signed-off-by: Nils Bandener <[email protected]> --------- Signed-off-by: Nils Bandener <[email protected]>
liuguoqingfz
pushed a commit
to liuguoqingfz/OpenSearch
that referenced
this pull request
Dec 15, 2025
…being deserialized from TransportRequest stream (opensearch-project#20132) * BulkRequest: Make sure that indices Set is properly initialized when being deserialized from TransportRequest stream Signed-off-by: Nils Bandener <[email protected]> * fix Signed-off-by: Nils Bandener <[email protected]> * simplified BytesStreamOutput construction Signed-off-by: Nils Bandener <[email protected]> --------- Signed-off-by: Nils Bandener <[email protected]>
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.
Description
In the class
BulkRequest, theindicesproperty provides a short-cut to retrieve the names of all indices referenced in the bulk items.The index names are added to the
Setwhen one of theadd()methods is being called. However, the property is neither being serialized, nor the constructor for deserialization makes sure that the property is being properly initialized again. Thus, the property will be an empty set after the request was serialized and deserialized.Fortunately, this happens only in rare cases, specifically when ingestion pipelines are present and the request did not hit a node with the role "ingest". Compare this code:
OpenSearch/server/src/main/java/org/opensearch/action/bulk/TransportBulkAction.java
Lines 254 to 277 in 1ee30dc
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
Bug Fixes
Tests
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.