Removes max_encoding_message_size from all grpc servers and clients #4135
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
Test Results 7 files + 2 7 suites +2 3m 14s ⏱️ + 2m 11s Results for commit 6220e4f. ± Comparison against base commit f181056. This pull request removes 18 and adds 47 tests. Note that renamed tests count towards both.♻️ This comment has been updated with latest results. |
AhmedSoliman
changed the title
This change is part of #4130 1. Errors in the send path caused by encoding limits cannot be surfaced in logs or handled properly - connections are silently dropped. We leave the encoding limit as unlimited (the default) and will add explicit checks in key parts of the send path in a subsequent PR. 2. Setting a high decode limit does not add overhead since tonic does not preallocate this value per decoder. It only uses it as a check to limit uncompressed buffer sizes. Decode failures due to limits are correctly surfaced in logs. Therefore, we are opting to have an unlimited encoding limit on the grpc layer since failures are obsecure and we'll selectively add explicit checks in the important parts of the send path in a subsequent PR. The decode size limit is still set so practically, the impact of this change is improved logging when the limit is hit.
AhmedSoliman
changed the title
AhmedSoliman
changed the title
There was a problem hiding this comment.
Pull request overview
This PR removes max_encoding_message_size configuration from all gRPC servers and clients while retaining the max_decoding_message_size limit. The change improves error visibility by addressing the issue where encoding limit failures silently drop connections without proper logging, while decode limits correctly surface errors in logs.
Key changes:
- Removed
max_encoding_message_sizecalls from all gRPC server implementations - Removed
max_encoding_message_sizecalls from all gRPC client implementations - Retained
max_decoding_message_sizeconfiguration across all servers and clients
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| crates/node/src/network_server/grpc_svc_handler.rs | Removed encoding size limits from NodeCtlSvcHandler and MetadataProxySvcHandler servers |
| crates/metadata-store/src/protobuf.rs | Removed encoding size limit from MetadataProxySvcClient |
| crates/metadata-server/src/raft/network/handler.rs | Removed encoding size limit from MetadataServerNetworkHandler server |
| crates/metadata-server/src/grpc/handler.rs | Removed encoding size limit from MetadataServerHandler server |
| crates/metadata-server-grpc/src/grpc.rs | Removed encoding size limit from MetadataServerSvcClient |
| crates/log-server/src/protobuf.rs | Removed encoding size limit from log_server_client |
| crates/log-server/src/grpc_svc_handler.rs | Removed encoding size limit from LogServerSvcServer |
| crates/log-server-grpc/src/lib.rs | Removed encoding size limit from log_server_client |
| crates/core/src/protobuf.rs | Removed encoding size limits from ClusterCtrlSvcClient and NodeCtlSvcClient |
| crates/core/src/network/grpc/svc_handler.rs | Removed encoding size limit from CoreNodeSvcHandler server |
| crates/core/src/network/grpc/connector.rs | Removed encoding size limit from CoreNodeSvcClient |
| crates/admin/src/cluster_controller/grpc_svc_handler.rs | Removed encoding size limit from ClusterCtrlSvcHandler server |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
This change is part of #4130
leave the encoding limit as unlimited (the default) and will add explicit checks in key parts of the send path in a subsequent PR.
Therefore, we are opting to have an unlimited encoding limit on the grpc layer since failures are obsecure and we'll
selectively add explicit checks in the important parts of the send path in a subsequent PR.
The decode size limit is still set so practically, the impact of this change is improved logging when the limit is hit.
Stack created with Sapling. Best reviewed with ReviewStack.