Conversation
There was a problem hiding this comment.
Pull request overview
Adds a dedicated gRPC message size configuration for the Search service to prevent “encoded message length too large” errors when returning large result pages (e.g., 10k hits), while keeping the existing general gRPC message size setting for other services.
Changes:
- Introduces
grpc.max_search_message_sizeinGrpcConfig(default 60MiB) alongside the existinggrpc.max_message_size(default 20MiB). - Applies the new search-specific limit to the Search gRPC server’s message size settings.
- Uses the new search-specific limit when creating Search gRPC clients in the searcher pool wiring.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| quickwit/quickwit-serve/src/lib.rs | Switches search client message size to use max_search_message_size. |
| quickwit/quickwit-serve/src/grpc.rs | Configures SearchServiceServer max encode/decode message size using max_search_message_size. |
| quickwit/quickwit-config/src/node_config/mod.rs | Adds max_search_message_size to GrpcConfig, defaulting and tests updated to include the new field. |
Comments suppressed due to low confidence (3)
quickwit/quickwit-config/src/node_config/mod.rs:120
GrpcConfig::validateonly checksgrpc.max_message_size, but the newgrpc.max_search_message_sizecan still be set to an unusably small value and will then be applied to the search gRPC client/server limits. Add a similar validation formax_search_message_size(e.g. >= 1MB) and update the error message to mentiongrpc.max_search_message_size.
pub fn validate(&self) -> anyhow::Result<()> {
ensure!(
self.max_message_size >= ByteSize::mb(1),
"max gRPC message size (`grpc.max_message_size`) must be at least 1MB, got `{}`",
self.max_message_size
);
Ok(())
quickwit/quickwit-config/src/node_config/mod.rs:870
test_grpc_config_validatedoesn’t cover validation ofmax_search_message_size(the failing case still fails becausemax_message_sizeis too small). Add a test case wheremax_message_sizeis valid butmax_search_message_sizeis below the minimum, and assert thatvalidate()errors.
fn test_grpc_config_validate() {
let grpc_config = GrpcConfig {
max_message_size: ByteSize::mb(1),
max_search_message_size: ByteSize::mb(1),
tls: None,
keep_alive: None,
};
assert!(grpc_config.validate().is_ok());
let grpc_config = GrpcConfig {
max_message_size: ByteSize::kb(1),
max_search_message_size: ByteSize::kb(1),
tls: None,
keep_alive: None,
};
assert!(grpc_config.validate().is_err());
}
quickwit/quickwit-serve/src/lib.rs:1055
- In
setup_searcher,max_message_sizenow actually refers togrpc.max_search_message_size. Renaming this local variable to something search-specific (e.g.max_search_message_size) would avoid confusion, especially since the same file also usesmax_message_sizefor other gRPC clients.
let max_message_size = node_config.grpc_config.max_search_message_size;
let searcher_change_stream = cluster_change_stream.filter_map(move |cluster_change| {
let search_service_clone = search_service_clone.clone();
Box::pin(async move {
match cluster_change {
ClusterChange::Add(node) if node.is_searcher() => {
let chitchat_id = node.chitchat_id();
info!(
node_id = chitchat_id.node_id,
generation_id = chitchat_id.generation_id,
"adding node `{}` to searcher pool",
chitchat_id.node_id,
);
let grpc_addr = node.grpc_advertise_addr();
if node.is_self_node() {
let search_client =
SearchServiceClient::from_service(search_service_clone, grpc_addr);
Some(Change::Insert(grpc_addr, search_client))
} else {
// Set a very high timeout just to be sure that we cleanup at some point.
let timeout_channel = Timeout::new(node.channel(), Duration::from_mins(30));
let search_client = create_search_client_from_channel(
grpc_addr,
timeout_channel,
max_message_size,
);
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| let max_message_size_bytes = grpc_config.max_search_message_size.0 as usize; | ||
| Some( | ||
| SearchServiceServer::new(grpc_search_service) | ||
| .max_decoding_message_size(grpc_config.max_message_size.0 as usize) | ||
| .max_encoding_message_size(grpc_config.max_message_size.0 as usize), | ||
| .max_decoding_message_size(max_message_size_bytes) | ||
| .max_encoding_message_size(max_message_size_bytes), | ||
| ) |
There was a problem hiding this comment.
The search service is configured with the same max_search_message_size for both decoding and encoding. If the intent is only to allow larger responses, this also increases the maximum accepted request size for the search endpoint, which can have DoS/memory implications. Consider keeping max_decoding_message_size at grpc.max_message_size and only increasing max_encoding_message_size for search responses (and ensure the client-side limits mirror this).
Description
Avoid Error, encoded message length too large when requesting large result pages (10k).
How was this PR tested?
Describe how you tested this PR.