Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

allow configuring grpc max receive message size #738

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jasonwbarnett
Copy link

@jasonwbarnett jasonwbarnett commented Mar 15, 2024

Fixes #658

@jasonwbarnett jasonwbarnett force-pushed the jwb/add-flag-to-configure-grpc-max-recv-msg-size branch 5 times, most recently from a5a4e79 to 6a4661f Compare March 15, 2024 16:01
@jasonwbarnett jasonwbarnett force-pushed the jwb/add-flag-to-configure-grpc-max-recv-msg-size branch from 6a4661f to 920f2cf Compare March 15, 2024 16:18
@@ -393,6 +393,8 @@ func startGrpcServer(c *config.Config, grpcServer **grpc.Server,

opts = append(opts, grpc.ChainStreamInterceptor(streamInterceptors...))
opts = append(opts, grpc.ChainUnaryInterceptor(unaryInterceptors...))
log.Println("Setting gRPC Max Receive Message Size to:", c.GrpcMaxRecvMsgSize)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this line left over from debugging?

Copy link
Author

@jasonwbarnett jasonwbarnett Apr 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, this was left intentionally as it is generally helpful to communicate upon startup what the picked up/configured value is. Especially useful if two values are provided, one at the CLI and the other in the config.

@buchgr
Copy link
Owner

buchgr commented Apr 4, 2024

Seems like Pants is implementing a principled fix in pantsbuild/pants#20708. How do you want to proceeed with this PR @jasonwbarnett ?

@jasonwbarnett
Copy link
Author

jasonwbarnett commented Apr 5, 2024

@buchgr It's up to you. I don't know much about gRPC and when it is or isn't appropriate to increase the default message send/receive sizes. If there are cases where it's appropriate, I'd like to see this get merged. If there aren't ever scenarios where this is appropriate, it should probably just be closed without a merge.

@mostynb
Copy link
Collaborator

mostynb commented Apr 7, 2024

I think if we decide to land this PR, then the flag needs to have a prominent warning about ensuring that the client side configuration matches for all clients that use the cache.

However, if pants only needs this for FindMissingBlobs, then that should be an easy fix on the pants side and we should not add this option to bazel-remote.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow setting max received message size
4 participants