albertobastos commented on code in PR #15208:
URL: https://github.com/apache/pinot/pull/15208#discussion_r1984646907
##########
pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/MultiStageBrokerRequestHandler.java:
##########
@@ -309,18 +309,19 @@ protected BrokerResponse handleRequest(long requestId,
String query, SqlNodeAndO
return new BrokerResponseNative(QueryException.EXECUTION_TIMEOUT_ERROR);
}
- String clientRequestId = extractClientRequestId(sqlNodeAndOptions);
- onQueryStart(requestId, clientRequestId, query);
-
try {
Tracing.ThreadAccountantOps.setupRunner(String.valueOf(requestId),
ThreadExecutionContext.TaskType.MSE);
long executionStartTimeNs = System.nanoTime();
QueryDispatcher.QueryResult queryResults;
+ String clientRequestId = extractClientRequestId(sqlNodeAndOptions);
+ //onQueryStart(requestId, clientRequestId, query);
Review Comment:
The `onQueryStart()` was a bad name choice, cause it is expected to be run
when the query is actually running (meaning: already sent to the servers). If
we do it otherwise, there's a risk window where a query is already returned by
the `/queries` endpoint, but the Broker has not sent it yet to the servers or,
even worse, still doesn't know what servers will forward the query to.
In a "human" scenario the risk of the Cancellation API being unreliable was
smaller, cause this situation only emerges if the cancellation request is sent
immediately the moment the query is returned by the `/queries` endpoint. In our
unit tests, thought, that's exactly what happened so we had a flaky test where
the Broker received a valid cancellation request but wasn't able to forward it
to any servers.
I did a little refactor changing the method to `onQueryRunning()` and
calling it where it actually should, both for the SSE and MSE. Please check it
out.
31a42cc18a872ab1417b5ddc7c618bc89b805e82
@vrajat You already detected something about that and added it as a comment,
please check if the changes suit you.
##########
pinot-query-runtime/src/main/java/org/apache/pinot/query/service/dispatch/QueryDispatcher.java:
##########
@@ -142,10 +142,19 @@ public void start() {
public QueryResult submitAndReduce(RequestContext context,
DispatchableSubPlan dispatchableSubPlan, long timeoutMs,
Map<String, String> queryOptions)
throws Exception {
+ return submitAndReduce(context, dispatchableSubPlan, timeoutMs,
queryOptions, null);
+ }
+
+ public QueryResult submitAndReduce(RequestContext context,
DispatchableSubPlan dispatchableSubPlan, long timeoutMs,
+ Map<String, String> queryOptions, Runnable beforeReduce)
Review Comment:
Yeah, I tend to forget using it.
--
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
