[GRPC] Bulk optimization and fixes #19937
Conversation
…llsback to object for bwc) bytes optimize Set the default value of source to null to match REST Support allowExplicitIndex setting Signed-off-by: Karen Xu <[email protected]>
|
❌ Gradle check result for f3818ca: 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 Xu <[email protected]>
Signed-off-by: Karen Xu <[email protected]>
|
❕ Gradle check result for 34bbaf4: 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 Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #19937 +/- ##
============================================
- Coverage 73.27% 73.19% -0.09%
+ Complexity 71563 71546 -17
============================================
Files 5785 5785
Lines 326822 326853 +31
Branches 47294 47299 +5
============================================
- Hits 239484 239237 -247
- Misses 68111 68372 +261
- Partials 19227 19244 +17 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Signed-off-by: Karen Xu <[email protected]>
|
❌ Gradle check result for 6d897b8: 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? |
|
❌ Gradle check result for 6d897b8: 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? |
| index = createOperation.hasXIndex() ? createOperation.getXIndex() : index; | ||
| // Check explicit index (matches REST BulkRequestParser line 218-221) | ||
| if (createOperation.hasXIndex()) { | ||
| if (!allowExplicitIndex && defaultIndex != null) { |
There was a problem hiding this comment.
Does allowExplicitIndex have a use case in the context of gRPC? For REST one use case could be forcing users to provide the target index in the URI which is more easily filtered on than parsing a request body. For gRPC though this information will always be in the request body and allowExplicitIndex will only control which index fields you are allowed to use.
There was a problem hiding this comment.
Good point, let me remove it to decouple from this PR for now
I'm not sure of the original intent of allowExplicitIndex - is it just for security concerns or could it also help to reduce the sizes of request payloads? Erring on the side of caution, had added support for it in the gRPC APIs to maintain parity but I can create a separate issue to track it later: #19962
...a/org/opensearch/transport/grpc/proto/request/document/bulk/BulkRequestParserProtoUtils.java
Show resolved
Hide resolved
Signed-off-by: Karen Xu <[email protected]>
|
❌ Gradle check result for 2a16f30: 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? |
...a/org/opensearch/transport/grpc/proto/request/document/bulk/BulkRequestParserProtoUtils.java
Show resolved
Hide resolved
|
❕ Gradle check result for 2a16f30: 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. |
Signed-off-by: Karen Xu <[email protected]>
...a/org/opensearch/transport/grpc/proto/request/document/bulk/BulkRequestParserProtoUtils.java
Show resolved
Hide resolved
|
|
||
| if (buffer.hasArray()) { | ||
| // Fast path: ByteBuffer is backed by an array | ||
| byte[] array = buffer.array(); |
There was a problem hiding this comment.
Can we add a test to cover this path to confirm buffer.array() works fine on a read-only byte buffer?
Update: it might actually fail with ReadOnlyBufferException (ref)
There was a problem hiding this comment.
Out of curiosity any idea what cases we will not have an array backed buffer? And similarly when the array will have an offset?
| // Fallback: ByteBuffer is not array-backed (rare case) | ||
| // Must copy in this case | ||
| byte[] bytes = new byte[buffer.remaining()]; | ||
| buffer.get(bytes); |
There was a problem hiding this comment.
could we instead use ByteString.toByteArray()?
msfroh
left a comment
msfroh
left a comment
There was a problem hiding this comment.
Looks good to me overall. I really appreciate the use of zero-copy for the byte arrays.
I'll hold off on merging until you've had a chance to respond to @varunbharadwaj's comments.
...a/org/opensearch/transport/grpc/proto/request/document/bulk/BulkRequestParserProtoUtils.java
Show resolved
Hide resolved
4 participants
Description
A series of gRPC bulk API fixes/optimizations:
docfield instead ofobjectfield (still fallsback to object for backward compatibility)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.