Raise client gRPC message cap to 128 MiB#51
Closed
jnathangreeg wants to merge 1 commit into
Closed
Conversation
- Introduced maxGRPCMessageSize constant to set a higher limit for both send and receive operations, allowing larger payloads for unary RPCs. - Updated Connect method to apply the new message size limits in gRPC call options. This change improves the capability of the StorageClient to handle larger data transfers effectively.
📝 WalkthroughWalkthroughThis PR increases gRPC message size limits in the storage client. A new constant ChangesgRPC Message Size Limits
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/client/v1/storageclient.go (1)
263-267: ⚡ Quick winUpdate deprecation comments to reflect new 128 MiB limit.
The deprecation comments for
SendContainerProfile(lines 263-267) andGetContainerProfile(lines 492-496) state that unary methods are "capped at gRPC's default 4 MiB message size." With this PR raising the client limit to 128 MiB, these comments are now misleading.Consider updating them to:
- Clarify that the client now supports up to 128 MiB for unary RPCs (when the server is also configured)
- Retain the recommendation to use streaming for very large profiles, but adjust the threshold or rationale
- Or simply note that streaming is preferred for arbitrarily large payloads without mentioning a specific cap
📝 Proposed update for deprecation comments
For
SendContainerProfile(lines 263-267):// SendContainerProfile sends a container profile to the storage server. // // Deprecated: use SendContainerProfileStream. The unary form is silently -// capped at gRPC's default 4 MiB message size; profiles with many or -// large entries can exceed this and fail on the wire. The streaming -// variant has no such bound. +// capped at the configured gRPC message size (128 MiB); very large profiles +// can exceed this and fail on the wire. The streaming variant has no such +// bound and is preferred for arbitrarily large payloads.For
GetContainerProfile(lines 492-496):// GetContainerProfile retrieves a ContainerProfile from the storage server. // // Deprecated: use GetContainerProfileStream. The unary form goes through -// GetProfile, which is capped at gRPC's default 4 MiB message size; -// profiles with many or large entries can exceed this and fail on the -// wire. The streaming variant has no such bound. +// GetProfile, which is capped at the configured gRPC message size (128 MiB); +// very large profiles can exceed this and fail on the wire. The streaming +// variant has no such bound and is preferred for arbitrarily large payloads.Also applies to: 492-496
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/client/v1/storageclient.go` around lines 263 - 267, The deprecation comments for SendContainerProfile and GetContainerProfile still say unary RPCs are "capped at gRPC's default 4 MiB", which is no longer true; update the doc comments on the SendContainerProfile and GetContainerProfile methods to either state the client now supports up to 128 MiB for unary calls (when the server is likewise configured) and recommend streaming only for larger/very large profiles, or remove the 4 MiB figure and instead state that streaming is preferred for arbitrarily large payloads; ensure you reference the method names SendContainerProfile and GetContainerProfile in the updated comments so readers can find them.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@pkg/client/v1/storageclient.go`:
- Around line 263-267: The deprecation comments for SendContainerProfile and
GetContainerProfile still say unary RPCs are "capped at gRPC's default 4 MiB",
which is no longer true; update the doc comments on the SendContainerProfile and
GetContainerProfile methods to either state the client now supports up to 128
MiB for unary calls (when the server is likewise configured) and recommend
streaming only for larger/very large profiles, or remove the 4 MiB figure and
instead state that streaming is preferred for arbitrarily large payloads; ensure
you reference the method names SendContainerProfile and GetContainerProfile in
the updated comments so readers can find them.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: eaf8d2b0-7f88-4bce-b180-7af2583e3cce
📒 Files selected for processing (1)
pkg/client/v1/storageclient.go
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
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.
Summary
maxGRPCMessageSize = 128 << 20constant inpkg/client/v1/storageclient.go.grpc.MaxCallRecvMsgSize/grpc.MaxCallSendMsgSizeviagrpc.WithDefaultCallOptionsinConnect().Why
grpc-go defaults the receive cap to 4 MiB on both peers. Larger unary downloads (
GetProfile,GetContainerProfile,GetApplicationProfile, etc.) fail at the client withResourceExhausted: received message larger than max. Raising the client cap to 128 MiB lets those downloads complete.Note: this is the client-side half. The storage server's own
MaxRecvMsgSizemust also be raised for large unary uploads to succeed — a companion change is going out on the cadashboardbe side.Streaming RPCs (
PutSBOMStream,GetSBOMStream,SendContainerProfileStream,GetContainerProfileStream) are unaffected; chunks remain 1 MiB.Test plan
go test ./pkg/client/v1/...).GetContainerProfilewith a >4 MiB profile and confirm it no longer returnsResourceExhausted.Summary by CodeRabbit