xiangfu0 commented on code in PR #19982:
URL: https://github.com/apache/pinot/pull/19982#discussion_r4506718829
##########
pinot-core/src/main/java/org/apache/pinot/core/transport/QueryRouter.java:
##########
@@ -460,6 +460,22 @@ void receiveDataTable(ServerRoutingInstance
serverRoutingInstance, DataTable dat
}
}
+ // Unlike receiveDataTable, a deserialization failure cannot be tied to a
single requestId (the bytes didn't
+ // parse), so this method must broadcast to every in-flight query on the
server channel. This is consistent with
+ // markServerUnavailable and safe because the channel is closed immediately
after.
+ void markServerDeserializationError(ServerRoutingInstance
serverRoutingInstance, Exception exception,
Review Comment:
MINOR: this no-channel overload has no callers. If something called it
later, the broadcast would skip the channel scoping (`channel == null` matches
every request). Consider removing it. The no-channel
`AsyncQueryResponse.receiveDeserializationError` overload is likewise only used
by tests.
---
AI-generated review via IntentLab · Model: Claude Opus 5.5 (claude-opus-5-5).
##########
pinot-core/src/main/java/org/apache/pinot/core/transport/AsyncQueryResponse.java:
##########
@@ -214,8 +216,36 @@ void markRequestSent(ServerRoutingInstance
serverRoutingInstance, int requestSen
_responseMap.get(serverRoutingInstance).markRequestSent(requestSentLatencyMs);
}
+ // Handle server DataTable deserialization errors without waiting for query
timeout
+ void receiveDeserializationError(ServerRoutingInstance
serverRoutingInstance, Exception exception,
+ int responseSize, int deserializationTimeMs) {
+ receiveDeserializationError(serverRoutingInstance, null, exception,
responseSize, deserializationTimeMs);
+ }
+
+ void receiveDeserializationError(ServerRoutingInstance
serverRoutingInstance, @Nullable Channel channel,
+ Exception exception, int responseSize, int deserializationTimeMs) {
+ if (!shouldActOnServerDown(serverRoutingInstance, channel)) {
+ return;
+ }
+ if (!_countedDownServers.add(serverRoutingInstance)) {
+ return;
+ }
+ _failedServer = serverRoutingInstance;
+ DataTable dataTable = DataTableBuilderFactory.getEmptyDataTable();
+ dataTable.getMetadata().put(DataTable.MetadataKey.REQUEST_ID.getName(),
Long.toString(_requestId));
+ dataTable.addException(QueryErrorCode.DESERIALIZATION_ERROR.getId(),
+ exception.getMessage() != null ? exception.getMessage() :
exception.toString());
+ ServerResponse response = _responseMap.get(serverRoutingInstance);
+ response.receiveDeserializationError(dataTable, responseSize,
deserializationTimeMs);
+ _numServersResponded.getAndIncrement();
Review Comment:
MINOR (semantics, for the owner): the synthetic 435 table counts toward
`numServersResponded`, and `doScatter` treats it as a response. So
`SERVER_NOT_RESPONDING` and `BROKER_RESPONSES_WITH_PARTIAL_SERVERS_RESPONDED`
are not emitted for queries cut off on this channel. If that's intended, a
short comment would help.
---
AI-generated review via IntentLab · Model: Claude Opus 5.5 (claude-opus-5-5).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
