You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "gortiz (via GitHub)" <gi...@apache.org> on 2025/06/11 13:10:00 UTC
[PR] Introduce MSE active and passive timeouts [pinot]
gortiz opened a new pull request, #16075:
URL: https://github.com/apache/pinot/pull/16075
Given that all workers were using the same deadtime, any worker could fail. Due to the transmission relay, workers running stages closer to the root have a higher chance of producing the message that ultimately reaches the broker and, therefore, is used to report the error.
This PR solves this problem by adding two different deadlines:
- Active deadline, the one that users specified so far, which is used whenever we have to wait for some _cpu_ work. For example, a join building the final block.
- Passive deadline, a larger timeout than the active deadline. This is the timeout used when we need to wait for another stage, like when a mailbox receive operator waits for more data.
The active deadline can be customized in the same way we are used to. The passive timeout can be configured:
- With the new query option `extraPassiveTimeoutMs`, which is the extra number of ms we want to wait on passive waiters. This option is not required.
- When the query option `extraPassiveTimeoutMs` is not provided, brokers use a default timeout. This default can be changed with the new broker configuration `pinot.broker.passiveTimeoutMs`, which defaults to 100ms
To be clear, in normal situations, this PR doesn't increase the normal timeout. It divides the timeout errors in two different types: active and passive and makes active timeouts more likely to reach the broker before the passive ones, which means we end up reporting the actual error message. This also reduces the warning logs printed when we try to send a message and the grpc connection has already timed out. Passive timeouts should only be reached when the network is extremely slow or when the extra passive timeout is set to 0.
--
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Introduce MSE active and passive timeouts [pinot]
Posted by "gortiz (via GitHub)" <gi...@apache.org>.
gortiz merged PR #16075:
URL: https://github.com/apache/pinot/pull/16075
--
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Introduce MSE active and passive timeouts [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on code in PR #16075:
URL: https://github.com/apache/pinot/pull/16075#discussion_r2258537648
##########
pinot-query-runtime/src/main/java/org/apache/pinot/query/service/dispatch/QueryDispatcher.java:
##########
@@ -452,6 +452,8 @@ private static Map<String, String> prepareRequestMetadata(long requestId, String
requestMetadata.put(CommonConstants.Query.Request.MetadataKeys.CORRELATION_ID, cid);
requestMetadata.put(CommonConstants.Broker.Request.QueryOptionKey.TIMEOUT_MS,
Long.toString(deadline.timeRemaining(TimeUnit.MILLISECONDS)));
+ requestMetadata.put(CommonConstants.Broker.Request.QueryOptionKey.EXTRA_PASSIVE_TIMEOUT_MS,
+ Long.toString(QueryThreadContext.getPassiveDeadlineMs()));
Review Comment:
This is a critical bug where we accidentally put absolute time as delta. Fixed in #16530
--
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Introduce MSE active and passive timeouts [pinot]
Posted by "gortiz (via GitHub)" <gi...@apache.org>.
gortiz commented on PR #16075:
URL: https://github.com/apache/pinot/pull/16075#issuecomment-2982921299
> I'm curious to know why this should be a query option and how is it intended to be used in practice? If this is only to improve error reporting for query failures, shouldn't we just automatically introduce some additional "passive" timeout internally (maybe based on some small %age of the actual query timeout)?
We use a default value of 100 ms, but this can be adjusted with query options. I think that could be useful in production environments where the 100ms is not sufficient (i.e., queries where the stage tree is very deep). But even if it isn't, it is super useful to have this query option to verify the feature works on a quickstart :smile:
--
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Introduce MSE active and passive timeouts [pinot]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #16075:
URL: https://github.com/apache/pinot/pull/16075#issuecomment-2983083432
## [Codecov](https://app.codecov.io/gh/apache/pinot/pull/16075?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
Attention: Patch coverage is `2.17391%` with `90 lines` in your changes missing coverage. Please review.
> Project coverage is 33.27%. Comparing base [(`1a476de`)](https://app.codecov.io/gh/apache/pinot/commit/1a476dea72ebf1c15e14ded28ae60c385f71b2c3?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) to head [(`bbd91a2`)](https://app.codecov.io/gh/apache/pinot/commit/bbd91a23f266f4ccbf361e24fcd3e597e58fca44?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
> Report is 291 commits behind head on master.
| [Files with missing lines](https://app.codecov.io/gh/apache/pinot/pull/16075?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [...org/apache/pinot/spi/query/QueryThreadContext.java](https://app.codecov.io/gh/apache/pinot/pull/16075?src=pr&el=tree&filepath=pinot-spi%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fspi%2Fquery%2FQueryThreadContext.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvcXVlcnkvUXVlcnlUaHJlYWRDb250ZXh0LmphdmE=) | 0.00% | [30 Missing :warning: ](https://app.codecov.io/gh/apache/pinot/pull/16075?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...ot/query/runtime/plan/OpChainExecutionContext.java](https://app.codecov.io/gh/apache/pinot/pull/16075?src=pr&el=tree&filepath=pinot-query-runtime%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fquery%2Fruntime%2Fplan%2FOpChainExecutionContext.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtcXVlcnktcnVudGltZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvcXVlcnkvcnVudGltZS9wbGFuL09wQ2hhaW5FeGVjdXRpb25Db250ZXh0LmphdmE=) | 0.00% | [12 Missing :warning: ](https://app.codecov.io/gh/apache/pinot/pull/16075?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...va/org/apache/pinot/query/runtime/QueryRunner.java](https://app.codecov.io/gh/apache/pinot/pull/16075?src=pr&el=tree&filepath=pinot-query-runtime%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fquery%2Fruntime%2FQueryRunner.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtcXVlcnktcnVudGltZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvcXVlcnkvcnVudGltZS9RdWVyeVJ1bm5lci5qYXZh) | 0.00% | [9 Missing :warning: ](https://app.codecov.io/gh/apache/pinot/pull/16075?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [.../pinot/query/service/dispatch/QueryDispatcher.java](https://app.codecov.io/gh/apache/pinot/pull/16075?src=pr&el=tree&filepath=pinot-query-runtime%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fquery%2Fservice%2Fdispatch%2FQueryDispatcher.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtcXVlcnktcnVudGltZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvcXVlcnkvc2VydmljZS9kaXNwYXRjaC9RdWVyeURpc3BhdGNoZXIuamF2YQ==) | 0.00% | [8 Missing :warning: ](https://app.codecov.io/gh/apache/pinot/pull/16075?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...requesthandler/MultiStageBrokerRequestHandler.java](https://app.codecov.io/gh/apache/pinot/pull/16075?src=pr&el=tree&filepath=pinot-broker%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fbroker%2Frequesthandler%2FMultiStageBrokerRequestHandler.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcmVxdWVzdGhhbmRsZXIvTXVsdGlTdGFnZUJyb2tlclJlcXVlc3RIYW5kbGVyLmphdmE=) | 0.00% | [6 Missing :warning: ](https://app.codecov.io/gh/apache/pinot/pull/16075?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...che/pinot/query/runtime/operator/LeafOperator.java](https://app.codecov.io/gh/apache/pinot/pull/16075?src=pr&el=tree&filepath=pinot-query-runtime%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fquery%2Fruntime%2Foperator%2FLeafOperator.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtcXVlcnktcnVudGltZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvcXVlcnkvcnVudGltZS9vcGVyYXRvci9MZWFmT3BlcmF0b3IuamF2YQ==) | 0.00% | [5 Missing :warning: ](https://app.codecov.io/gh/apache/pinot/pull/16075?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...not/query/runtime/operator/MultiStageOperator.java](https://app.codecov.io/gh/apache/pinot/pull/16075?src=pr&el=tree&filepath=pinot-query-runtime%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fquery%2Fruntime%2Foperator%2FMultiStageOperator.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtcXVlcnktcnVudGltZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvcXVlcnkvcnVudGltZS9vcGVyYXRvci9NdWx0aVN0YWdlT3BlcmF0b3IuamF2YQ==) | 0.00% | [5 Missing :warning: ](https://app.codecov.io/gh/apache/pinot/pull/16075?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...runtime/plan/pipeline/PipelineBreakerExecutor.java](https://app.codecov.io/gh/apache/pinot/pull/16075?src=pr&el=tree&filepath=pinot-query-runtime%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fquery%2Fruntime%2Fplan%2Fpipeline%2FPipelineBreakerExecutor.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtcXVlcnktcnVudGltZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvcXVlcnkvcnVudGltZS9wbGFuL3BpcGVsaW5lL1BpcGVsaW5lQnJlYWtlckV4ZWN1dG9yLmphdmE=) | 0.00% | [4 Missing :warning: ](https://app.codecov.io/gh/apache/pinot/pull/16075?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...ot/query/runtime/operator/MailboxSendOperator.java](https://app.codecov.io/gh/apache/pinot/pull/16075?src=pr&el=tree&filepath=pinot-query-runtime%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fquery%2Fruntime%2Foperator%2FMailboxSendOperator.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtcXVlcnktcnVudGltZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvcXVlcnkvcnVudGltZS9vcGVyYXRvci9NYWlsYm94U2VuZE9wZXJhdG9yLmphdmE=) | 0.00% | [3 Missing :warning: ](https://app.codecov.io/gh/apache/pinot/pull/16075?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...ry/runtime/plan/server/ServerPlanRequestUtils.java](https://app.codecov.io/gh/apache/pinot/pull/16075?src=pr&el=tree&filepath=pinot-query-runtime%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fquery%2Fruntime%2Fplan%2Fserver%2FServerPlanRequestUtils.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtcXVlcnktcnVudGltZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvcXVlcnkvcnVudGltZS9wbGFuL3NlcnZlci9TZXJ2ZXJQbGFuUmVxdWVzdFV0aWxzLmphdmE=) | 0.00% | [3 Missing :warning: ](https://app.codecov.io/gh/apache/pinot/pull/16075?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| ... and [3 more](https://app.codecov.io/gh/apache/pinot/pull/16075?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | |
> :exclamation: There is a different number of reports uploaded between BASE (1a476de) and HEAD (bbd91a2). Click for more details.
>
> <details><summary>HEAD has 68 uploads less than BASE</summary>
>
>| Flag | BASE (1a476de) | HEAD (bbd91a2) |
>|------|------|------|
>|java-21|10|1|
>|unittests|6|1|
>|unittests1|3|0|
>|skip-bytebuffers-true|4|0|
>|temurin|15|1|
>|unittests2|3|1|
>|skip-bytebuffers-false|8|0|
>|java-11|5|0|
>|integration|9|0|
>|integration2|3|0|
>|integration1|3|0|
>|custom-integration1|3|0|
></details>
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #16075 +/- ##
=============================================
- Coverage 62.90% 33.27% -29.63%
+ Complexity 1386 655 -731
=============================================
Files 2867 2951 +84
Lines 163354 169688 +6334
Branches 24952 25946 +994
=============================================
- Hits 102755 56470 -46285
- Misses 52847 108675 +55828
+ Partials 7752 4543 -3209
```
| [Flag](https://app.codecov.io/gh/apache/pinot/pull/16075/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [custom-integration1](https://app.codecov.io/gh/apache/pinot/pull/16075/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [integration](https://app.codecov.io/gh/apache/pinot/pull/16075/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [integration1](https://app.codecov.io/gh/apache/pinot/pull/16075/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [integration2](https://app.codecov.io/gh/apache/pinot/pull/16075/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [java-11](https://app.codecov.io/gh/apache/pinot/pull/16075/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [java-21](https://app.codecov.io/gh/apache/pinot/pull/16075/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `33.27% <2.17%> (-29.55%)` | :arrow_down: |
| [skip-bytebuffers-false](https://app.codecov.io/gh/apache/pinot/pull/16075/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [skip-bytebuffers-true](https://app.codecov.io/gh/apache/pinot/pull/16075/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [temurin](https://app.codecov.io/gh/apache/pinot/pull/16075/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `33.27% <2.17%> (-29.63%)` | :arrow_down: |
| [unittests](https://app.codecov.io/gh/apache/pinot/pull/16075/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `33.27% <2.17%> (-29.63%)` | :arrow_down: |
| [unittests1](https://app.codecov.io/gh/apache/pinot/pull/16075/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [unittests2](https://app.codecov.io/gh/apache/pinot/pull/16075/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `33.27% <2.17%> (-0.30%)` | :arrow_down: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/pinot/pull/16075?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Introduce MSE active and passive timeouts [pinot]
Posted by "yashmayya (via GitHub)" <gi...@apache.org>.
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Introduce MSE active and passive timeouts [pinot]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #16075:
URL: https://github.com/apache/pinot/pull/16075#discussion_r2140095695
##########
pinot-query-runtime/src/main/java/org/apache/pinot/query/runtime/operator/MultiStageOperator.java:
##########
@@ -72,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
+ /// (see for example [MailboxSendOperator][org.apache.pinot.query.runtime.operator.MailboxSendOperator]).
protected void sampleAndCheckInterruption() {
+ sampleAndCheckInterruption(_context.getActiveDeadlineMs());
+ }
+
+ /// 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.
+ protected void sampleAndCheckInterruption(long deadlineMs) {
+ if (System.currentTimeMillis() - deadlineMs > 0) {
Review Comment:
Consider replacing the subtraction check with a more direct comparison (e.g. System.currentTimeMillis() >= deadlineMs) for clarity.
```suggestion
if (System.currentTimeMillis() >= deadlineMs) {
```
--
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org