fix: signal DataTable deserialization errors immediately instead of waiting for timeout (#18866) - #19172
Open
waterWang wants to merge 5 commits into
Open
Conversation
…aiting for timeout (apache#18866)
…aiting for timeout (apache#18866)
…aiting for timeout (apache#18866)
…aiting for timeout (apache#18866)
…aiting for timeout (apache#18866)
Jackie-Jiang
added
bug
Jackie-Jiang
reviewed
Aug 6, 2026
Jackie-Jiang
left a comment
Jackie-Jiang
left a comment
Contributor
There was a problem hiding this comment.
Nice fix. The only thing we need to discuss is should we count this as failed server, or treat it as regular exception
| /// query can complete with partial results from the remaining servers instead of waiting for the full timeout. | ||
| void receiveDataTableDeserializationError(ServerRoutingInstance serverRoutingInstance) { | ||
| ServerResponse response = _responseMap.get(serverRoutingInstance); | ||
| if (response != null && response.getDataTable() == null) { |
Contributor
There was a problem hiding this comment.
Could this every be null or having a data table?
Should we count this as a failed server, or form a DataTable wrapping the exception and reuse receiveDataTable() flow
| /// Error deserializing a DataTable response from a server. The server sent back bytes that the broker could not | ||
| /// deserialize, so the query will complete with partial results from the other servers instead of waiting for the | ||
| /// full timeout. | ||
| DATA_TABLE_DESERIALIZATION_ERROR(426, "DataTableDeserializationError", Response.Status.INTERNAL_SERVER_ERROR), |
Contributor
There was a problem hiding this comment.
(minor) Insert it in the right position
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
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.
Fixes #18866
Problem
When a Pinot server sends back a response that the broker cannot deserialize into a
DataTable, theDataTableHandlercatch block only logs the error and bumps a metric — it never signals the query. The query then blocks until the full broker timeout expires and returns a misleadingBROKER_TIMEOUTwith partial results.Side effects (beyond the timeout):
Fix
QueryErrorCode.java: AddDATA_TABLE_DESERIALIZATION_ERROR(426)to distinguish deserialization failures from generic internal errors and timeouts.AsyncQueryResponse.java: AddreceiveDataTableDeserializationError()— counts down the latch for this server only (without failing the entire query), records the failed server, and sets the exception. This allows the query to complete with partial results from the remaining healthy servers.QueryRouter.java: AddreceiveDataTableDeserializationError()— iterates over in-flight queries and marks any still waiting on this server.DataTableHandler.java: In the catch block, call_queryRouter.receiveDataTableDeserializationError()instead of silently logging.QueryRoutingTest.java: AddtestDataTableDeserializationError()— sends garbage bytes as the server response and verifies the query completes in under 1 second instead of waiting for the 10-second timeout.Testing
testDataTableDeserializationError()— sendsnew byte[]{0,1,2,3,4,5}as server response, verifies query completes in < 1s (not 10s timeout)