yashmayya commented on code in PR #16075:
URL: https://github.com/apache/pinot/pull/16075#discussion_r2153958048
##########
pinot-query-runtime/src/main/java/org/apache/pinot/query/service/dispatch/QueryDispatcher.java:
##########
@@ -577,15 +579,30 @@ private TimeSeriesDispatchClient
getOrCreateTimeSeriesDispatchClient(
return _timeSeriesDispatchClientMap.computeIfAbsent(key, k -> new
TimeSeriesDispatchClient(hostname, port));
}
+ private static QueryResult runReducerFromQueryThread(
Review Comment:
The name of this method is confusing, it's running on the caller thread
right? Should it be something like `runReducerFromQueryThreadContext` instead?
##########
pinot-query-runtime/src/main/java/org/apache/pinot/query/runtime/operator/MultiStageOperator.java:
##########
@@ -73,9 +73,29 @@ public MultiStageOperator(OpChainExecutionContext context) {
public abstract void registerExecution(long time, int numRows);
- // Samples resource usage of the operator. The operator should call this
function for every block of data or
- // assuming the block holds 10000 rows or more.
+ /// This method should be called periodically by the operator to check
whether the execution should be interrupted.
+ ///
+ /// This could happen when the request deadline is reached, or the thread
accountant decides to interrupt the query
+ /// due to resource constraints.
+ ///
+ /// Normally, callers should call [#sampleAndCheckInterruption(long
deadlineMs)] passing the correct deadline, but
+ /// given most operators use either the active or the passive deadline, this
method is provided as a convenience
+ /// method. By default, it uses the active deadline, which is the one that
should be used for most operators, but
+ /// if the operator is not actively waiting for data, it could override this
method to use the passive deadline
Review Comment:
> if the operator is not actively waiting for data, it could override this
method to use the passive deadline
I didn't get the reasoning here - we're using the passive deadline for both
the mailbox send and receive operators for instance, right?
##########
pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/BaseBrokerRequestHandler.java:
##########
@@ -85,6 +85,7 @@ public abstract class BaseBrokerRequestHandler implements
BrokerRequestHandler {
protected final Set<String> _trackedHeaders;
protected final BrokerRequestIdGenerator _requestIdGenerator;
protected final long _brokerTimeoutMs;
+ protected final long _brokerPassiveTimeoutMs;
Review Comment:
If this is only meant for use with MSE, should this be in
`MultiStageBrokerRequestHandler` instead?
##########
pinot-query-runtime/src/main/java/org/apache/pinot/query/runtime/operator/BaseMailboxReceiveOperator.java:
##########
@@ -131,6 +131,12 @@ protected void onEos() {
}
}
+ @Override
+ protected void sampleAndCheckInterruption() {
+ // mailbox receive operator uses passive deadline instead of the active one
Review Comment:
Might be useful to add _why_ it uses the passive instead of active deadline
here?
##########
pinot-query-runtime/src/main/java/org/apache/pinot/query/runtime/plan/server/ServerPlanRequestUtils.java:
##########
@@ -280,7 +280,9 @@ private static void updateQueryOptions(PinotQuery
pinotQuery, OpChainExecutionCo
pinotQuery.setQueryOptions(queryOptions);
}
queryOptions.put(CommonConstants.Broker.Request.QueryOptionKey.TIMEOUT_MS,
- Long.toString(executionContext.getDeadlineMs() -
System.currentTimeMillis()));
+ Long.toString(executionContext.getActiveDeadlineMs() -
System.currentTimeMillis()));
+
queryOptions.put(CommonConstants.Broker.Request.QueryOptionKey.EXTRA_PASSIVE_TIMEOUT_MS,
+ Long.toString(executionContext.getPassiveDeadlineMs() -
executionContext.getActiveDeadlineMs()));
Review Comment:
The passive timeout isn't being used in the single-stage engine right?
##########
pinot-spi/src/main/java/org/apache/pinot/spi/utils/CommonConstants.java:
##########
@@ -500,6 +502,7 @@ public static class Request {
public static class QueryOptionKey {
public static final String TIMEOUT_MS = "timeoutMs";
+ public static final String EXTRA_PASSIVE_TIMEOUT_MS =
"extraPassiveTimeoutMs";
Review Comment:
nit: shouldn't we use `extra` either in both the query option and the broker
config or in neither (since they're configuring the same "passive" timeout
value)?
--
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]
