You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "Jackie-Jiang (via GitHub)" <gi...@apache.org> on 2025/08/18 21:35:55 UTC
[PR] Fix query cancellation on op-chain not scheduled [pinot]
Jackie-Jiang opened a new pull request, #16625:
URL: https://github.com/apache/pinot/pull/16625
## Problem
Currently, when a query is cancelled, there is a race condition where new operator chains for that query might still get scheduled after the cancellation request. This can lead to unnecessary resource usage and unexpected behavior.
## Solution
This PR introduces a synchronization mechanism to prevent operator chains from being scheduled for cancelled queries:
1. **Added per-query read-write locks**: Implemented a lock array (1024 locks) using request ID hashing to synchronize scheduling and cancellation operations
2. **Added cancelled query cache**: Tracks cancelled queries with configurable size and expiration time
3. **Modified registration flow**: Before scheduling an operator chain, acquire a read lock and check if the query has been cancelled
4. **Modified cancellation flow**: Acquire a write lock and mark the query as cancelled in the cache
## Configuration
Added new configuration options:
- `pinot.server.query.cancelled.cache.size` (default: 1000) - Maximum number of cancelled queries to cache
- `pinot.server.query.cancelled.cache.ms` (default: 60000) - Cache expiration time in milliseconds
## Files Changed
- `OpChainSchedulerService.java`: Core logic for synchronized scheduling and cancellation
- `CommonConstants.java`: Added new configuration constants
## Testing
The fix ensures that operator chains are not scheduled for queries that have already been cancelled, preventing the race condition.
--
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] Fix query cancellation on op-chain not scheduled [pinot]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #16625:
URL: https://github.com/apache/pinot/pull/16625#issuecomment-3199168148
## [Codecov](https://app.codecov.io/gh/apache/pinot/pull/16625?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:white_check_mark: All modified and coverable lines are covered by tests.
:white_check_mark: Project coverage is 63.31%. Comparing base ([`285a18b`](https://app.codecov.io/gh/apache/pinot/commit/285a18b84427e7e8e7e60ea8970e9fc2d5d325da?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`c3ed5eb`](https://app.codecov.io/gh/apache/pinot/commit/c3ed5eb9f918c8da501022f1ba9070a150147a08?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #16625 +/- ##
============================================
+ Coverage 63.29% 63.31% +0.02%
Complexity 1379 1379
============================================
Files 3027 3027
Lines 176446 176468 +22
Branches 27074 27076 +2
============================================
+ Hits 111675 111730 +55
+ Misses 56200 56178 -22
+ Partials 8571 8560 -11
```
| [Flag](https://app.codecov.io/gh/apache/pinot/pull/16625/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/16625/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `100.00% <ø> (ø)` | |
| [integration](https://app.codecov.io/gh/apache/pinot/pull/16625/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `100.00% <ø> (ø)` | |
| [integration1](https://app.codecov.io/gh/apache/pinot/pull/16625/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `100.00% <ø> (ø)` | |
| [integration2](https://app.codecov.io/gh/apache/pinot/pull/16625/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `0.00% <ø> (ø)` | |
| [java-11](https://app.codecov.io/gh/apache/pinot/pull/16625/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `63.26% <100.00%> (-0.01%)` | :arrow_down: |
| [java-21](https://app.codecov.io/gh/apache/pinot/pull/16625/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `63.29% <100.00%> (+6.91%)` | :arrow_up: |
| [temurin](https://app.codecov.io/gh/apache/pinot/pull/16625/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `63.31% <100.00%> (+0.02%)` | :arrow_up: |
| [unittests](https://app.codecov.io/gh/apache/pinot/pull/16625/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `63.31% <100.00%> (+0.02%)` | :arrow_up: |
| [unittests1](https://app.codecov.io/gh/apache/pinot/pull/16625/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `56.45% <100.00%> (+0.03%)` | :arrow_up: |
| [unittests2](https://app.codecov.io/gh/apache/pinot/pull/16625/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `33.13% <0.00%> (-0.01%)` | :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/16625?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] Fix query cancellation on op-chain not scheduled [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang merged PR #16625:
URL: https://github.com/apache/pinot/pull/16625
--
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] Fix query cancellation on op-chain not scheduled [pinot]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #16625:
URL: https://github.com/apache/pinot/pull/16625#discussion_r2283553572
##########
pinot-query-runtime/src/main/java/org/apache/pinot/query/runtime/executor/OpChainSchedulerService.java:
##########
@@ -42,39 +45,75 @@
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
+
+/// TODO: Use CID to manage the queries
Review Comment:
[nitpick] This TODO comment is unrelated to the changes in this PR. Consider removing it or addressing it in a separate PR to maintain focus on the race condition fix.
```suggestion
```
##########
pinot-spi/src/main/java/org/apache/pinot/spi/utils/CommonConstants.java:
##########
@@ -1942,7 +1942,17 @@ public static class PlanVersions {
/// Max time to keep the op stats in the cache.
public static final String KEY_OF_OP_STATS_CACHE_EXPIRE_MS = "pinot.server.query.op.stats.cache.ms";
public static final int DEFAULT_OF_OP_STATS_CACHE_EXPIRE_MS = 60 * 1000;
+
+ /// Max number of cancelled queries to keep in the cache.
+ public static final String KEY_OF_CANCELLED_QUERY_CACHE_SIZE = "pinot.server.query.cancelled.cache.size";
+ public static final int DEFAULT_OF_CANCELLED_QUERY_CACHE_SIZE = 1000;
+
+ /// Max time to keep the cancelled queries in the cache.
+ public static final String KEY_OF_CANCELLED_QUERY_CACHE_EXPIRE_MS = "pinot.server.query.cancelled.cache.ms";
+ public static final int DEFAULT_OF_CANCELLED_QUERY_CACHE_EXPIRE_MS = 60 * 1000;
+
/// Timeout of the cancel request, in milliseconds.
+ /// TODO: This is used by the broker. Consider renaming it.
Review Comment:
[nitpick] This TODO comment is unrelated to the changes in this PR and appears to be leftover from previous work. Consider removing it or addressing it in a separate PR to keep this change focused.
```suggestion
```
##########
pinot-query-runtime/src/main/java/org/apache/pinot/query/runtime/executor/OpChainSchedulerService.java:
##########
@@ -113,11 +152,20 @@ public void runJob() {
}
}
});
- _opChainCache.put(operatorChain.getId(), operatorChain.getRoot());
- _submittedOpChainMap.put(operatorChain.getId(), scheduledFuture);
+ _opChainCache.put(opChainId, operatorChain.getRoot());
+ _submittedOpChainMap.put(opChainId, scheduledFuture);
}
public Map<Integer, MultiStageQueryStats.StageStats.Closed> cancel(long requestId) {
+ // Acquire write lock for the query to ensure that the query is not cancelled while scheduling the operator chain.
+ Lock writeLock = getQueryLock(requestId).writeLock();
+ writeLock.lock();
+ try {
+ _cancelledQueryCache.put(requestId, requestId);
Review Comment:
Using the requestId as both key and value in the cache is redundant and wastes memory. Consider using a simpler approach like `_cancelledQueryCache.put(requestId, Boolean.TRUE)` or a Set-like structure to only store the key.
```suggestion
_cancelledQueryCache.put(requestId, Boolean.TRUE);
```
--
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] Fix query cancellation on op-chain not scheduled [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on PR #16625:
URL: https://github.com/apache/pinot/pull/16625#issuecomment-3201715080
> LGTM. Shall we add a metrics to monitor lock contention to adjust array size if needed.
I think it should be fine given the synchronized part is very short
--
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] Fix query cancellation on op-chain not scheduled [pinot]
Posted by "gortiz (via GitHub)" <gi...@apache.org>.
gortiz commented on PR #16625:
URL: https://github.com/apache/pinot/pull/16625#issuecomment-3209504815
I think this part of the code is getting very difficult to read and maintain. This PR is fine, but I think we need to refactor this code in the future. It shouldn't have to be so complex to register a query!
--
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