jasperjiaguo commented on code in PR #14029:
URL: https://github.com/apache/pinot/pull/14029#discussion_r1774150575
##########
pinot-core/src/main/java/org/apache/pinot/core/transport/AsyncQueryResponse.java:
##########
@@ -96,18 +97,44 @@ public Map<ServerRoutingInstance, ServerResponse>
getFinalResponses()
// servers even if the query times out or if servers have not responded.
for (Map.Entry<ServerRoutingInstance, ServerResponse> entry :
_responseMap.entrySet()) {
ServerResponse response = entry.getValue();
-
- // ServerResponse returns -1 if responseDelayMs is not set. This
indicates that a response was not received
- // from the server. Hence we set the latency to the timeout value.
- long latency =
- (response != null && response.getResponseDelayMs() >= 0) ?
response.getResponseDelayMs() : _timeoutMs;
+ long latency;
+
+ if (hasServerNotResponded(response) ||
hasServerReturnedExceptions(response)) {
Review Comment:
Can we add some more comments in this PR to explain the logic related to
these ADSS parts? Just in case some one in the future wants to change this part.
##########
pinot-core/src/test/java/org/apache/pinot/core/transport/QueryRoutingTest.java:
##########
@@ -201,6 +203,176 @@ public void testInvalidResponse()
queryServer.shutDown();
}
+ @Test
+ public void testLatencyForQueryServerException()
+ throws Exception {
+ long requestId = 123;
+ DataTable dataTable = DataTableBuilderFactory.getEmptyDataTable();
+ dataTable.getMetadata().put(MetadataKey.REQUEST_ID.getName(),
Long.toString(requestId));
+ Exception exception = new UnsupportedOperationException("Caught
exception.");
+ ProcessingException processingException =
+ QueryException.getException(QueryException.SERVER_TABLE_MISSING_ERROR,
exception);
+ dataTable.addException(processingException);
+ byte[] responseBytes = dataTable.toBytes();
+ String serverId = SERVER_INSTANCE.getInstanceId();
+ // Start the server
+ QueryServer queryServer = getQueryServer(0, responseBytes);
+ queryServer.start();
+
+ // Send a query with ServerSide exception and check if the latency is set
to timeout value.
+ Double latencyBefore =
_serverRoutingStatsManager.fetchEMALatencyForServer(serverId);
+ AsyncQueryResponse asyncQueryResponse =
+ _queryRouter.submitQuery(requestId, "testTable", BROKER_REQUEST,
ROUTING_TABLE, null, null, 1_000L);
+ Map<ServerRoutingInstance, ServerResponse> response =
asyncQueryResponse.getFinalResponses();
+ assertEquals(response.size(), 1);
+ assertTrue(response.containsKey(OFFLINE_SERVER_ROUTING_INSTANCE));
+
+ _requestCount += 2;
+ waitForStatsUpdate(_requestCount);
+ Double latencyAfter =
_serverRoutingStatsManager.fetchEMALatencyForServer(serverId);
+
+ if (latencyBefore == null) {
+ // This means that no queries were run before this test. So we can just
make sure that latencyAfter is equal to
+ //666.334.
+ // This corresponds to the EWMA value when a latency timeout value of
1000 is set. Latency set to timeout value
+ //when server side exception occurs.
+ double serverEWMALatency = 666.334;
+ assertEquals(latencyAfter, serverEWMALatency);
Review Comment:
nit: can we use assertEquals(expected, actual, delta) for fp comparison?
--
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: commits-unsubscr...@pinot.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org
For additional commands, e-mail: commits-h...@pinot.apache.org
