You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "anuragrai16 (via GitHub)" <gi...@apache.org> on 2026/04/06 08:58:26 UTC
[PR] [pinot-server/ proactive-query-killing] add initial SPI implementation for supporting query killing based on … [pinot]
anuragrai16 opened a new pull request, #18102:
URL: https://github.com/apache/pinot/pull/18102
This PR introduces all core classes for a proactive, scan-cost-based query killing framework. No existing query execution path is modified in this PR. A follow-up PR will wire these into BaseOperator.checkTermination() and the operator instrumentation points.
**New classes:**
- `QueryScanCostContext` (pinot-spi) — LongAdder-based, thread-safe accumulator for numEntriesScannedInFilter, numDocsScanned, and
numEntriesScannedPostFilter. One instance per query, shared across segment worker threads.
- `QueryKillingStrategy` (pinot-core) — Interface for pluggable kill decisions. Includes shouldTerminate(), buildKillReport(),
getErrorCode(), and forQuery() for table-level threshold overrides.
- `ScanEntriesThresholdStrategy` (pinot-core) — Default strategy. Kills queries exceeding maxEntriesScannedInFilter or maxDocsScanned.
Threshold of Long.MAX_VALUE disables that metric. Includes a nested Factory that validates config at init and logs a warning if no
thresholds are set.
- `CompositeQueryKillingStrategy` (pinot-core) — Combines strategies with AND/OR semantics.
- `QueryKillingStrategyFactory` (pinot-core) — Factory interface for config-driven strategy loading. Custom strategies are plugged in by
setting accounting.scan.based.killing.strategy.factory.class.name to a factory class
- `QueryKillReport` (pinot-core) — Immutable snapshot of a kill event. Captures all metrics at creation time
Produces customer-facing error messages with actionable advice and structured internal log messages.
- `QueryKillingManager` (pinot-core) — Singleton manager. Builds strategy once at init via factory, delegates
shouldTerminate()/buildKillReport() to the strategy.
--
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] [pinot-server/ proactive-query-killing] (1/2) add initial SPI implementation for supporting query killing based on Scan Cost [pinot]
Posted by "shauryachats (via GitHub)" <gi...@apache.org>.
shauryachats commented on code in PR #18102:
URL: https://github.com/apache/pinot/pull/18102#discussion_r3145002299
##########
pinot-core/src/main/java/org/apache/pinot/core/query/killing/QueryKillingManager.java:
##########
@@ -0,0 +1,184 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.core.query.killing;
+
+import java.util.concurrent.atomic.AtomicReference;
+import javax.annotation.Nullable;
+import org.apache.pinot.common.metrics.ServerMeter;
+import org.apache.pinot.common.metrics.ServerMetrics;
+import org.apache.pinot.core.accounting.QueryMonitorConfig;
+import org.apache.pinot.core.query.killing.strategy.ScanEntriesThresholdStrategy;
+import org.apache.pinot.spi.config.table.QueryConfig;
+import org.apache.pinot.spi.query.QueryExecutionContext;
+import org.apache.pinot.spi.query.QueryScanCostContext;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+
+/**
+ * Central manager for scan-based query killing. Owns the guard rails and delegates the
+ * actual kill decision to a {@link QueryKillingStrategy}.
+ *
+ * The default factory is {@link ScanEntriesThresholdStrategy.Factory}, which reads
+ * scan thresholds from {@link QueryMonitorConfig}. Custom factories can be configured
+ * via {@code accounting.scan.based.killing.strategy.factory.class.name}.
+ *
+ */
+public class QueryKillingManager {
+ private static final Logger LOGGER = LoggerFactory.getLogger(QueryKillingManager.class);
+
+ private static volatile QueryKillingManager _instance;
+
+ private final AtomicReference<QueryMonitorConfig> _configRef;
+ private final ServerMetrics _serverMetrics;
+
+ /**
+ * Null if: killing is disabled, config is insufficient, or factory failed to load.
+ * Rebuilt when config changes via {@link #rebuildStrategy()}.
+ */
+ @Nullable
+ private volatile QueryKillingStrategy _strategy;
+
+ public QueryKillingManager(AtomicReference<QueryMonitorConfig> configRef, ServerMetrics serverMetrics) {
+ _configRef = configRef;
+ _serverMetrics = serverMetrics;
+ }
+
+ /**
+ * Initializes the singleton instance and builds the strategy from config.
+ * Called once during server startup.
+ */
+ public static void init(AtomicReference<QueryMonitorConfig> configRef, ServerMetrics serverMetrics) {
+ QueryKillingManager manager = new QueryKillingManager(configRef, serverMetrics);
+ manager.rebuildStrategy();
+ _instance = manager;
+ }
+
+ @Nullable
+ public static QueryKillingManager getInstance() {
+ return _instance;
+ }
+
+ /**
+ * Rebuilds the strategy from the current config. Called at init and when
+ * cluster config changes (via the same onChange path that rebuilds QueryMonitorConfig).
+ */
+ public void rebuildStrategy() {
+ QueryMonitorConfig config = _configRef.get();
+ if (config == null || !config.isScanBasedKillingEnabled()) {
+ _strategy = null;
+ return;
+ }
+
+ try {
+ QueryKillingStrategyFactory factory = loadFactory(config);
+ _strategy = factory.create(config);
+ if (_strategy == null) {
+ LOGGER.warn("Scan-based killing is enabled but strategy factory '{}' returned null — "
+ + "required configuration may be missing. Scan-based killing will be effectively disabled.",
+ factory.getName());
+ }
+ } catch (Exception e) {
+ LOGGER.error("Failed to initialize scan-based killing strategy. "
Review Comment:
We might want to build some alerting on top of this, right?
--
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] [pinot-server/ proactive-query-killing] (1/2) add initial SPI implementation for supporting query killing based on Scan Cost [pinot]
Posted by "anuragrai16 (via GitHub)" <gi...@apache.org>.
anuragrai16 commented on code in PR #18102:
URL: https://github.com/apache/pinot/pull/18102#discussion_r3125847072
##########
pinot-core/src/main/java/org/apache/pinot/core/accounting/QueryMonitorConfig.java:
##########
@@ -20,11 +20,21 @@
import java.util.Map;
import java.util.Set;
+import javax.annotation.Nullable;
import org.apache.pinot.spi.env.PinotConfiguration;
import org.apache.pinot.spi.utils.CommonConstants;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
public class QueryMonitorConfig {
+ private static final Logger LOGGER = LoggerFactory.getLogger(QueryMonitorConfig.class);
+
+ private static final Set<String> VALID_SCAN_KILLING_MODES = Set.of(
Review Comment:
Makes sense, updated
--
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] [pinot-server/ proactive-query-killing] (1/2) add initial SPI implementation for supporting query killing based on Scan Cost [pinot]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #18102:
URL: https://github.com/apache/pinot/pull/18102#issuecomment-4191558051
## [Codecov](https://app.codecov.io/gh/apache/pinot/pull/18102?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:x: Patch coverage is `84.07080%` with `36 lines` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 56.11%. Comparing base ([`36cdab4`](https://app.codecov.io/gh/apache/pinot/commit/36cdab4e44949f5455f96863fcc72c5b6eb56f2f?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`36a6c2b`](https://app.codecov.io/gh/apache/pinot/commit/36a6c2bcb34a67834d7064f7f81674ef4828392a?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 3 commits behind head on master.
| [Files with missing lines](https://app.codecov.io/gh/apache/pinot/pull/18102?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [...ache/pinot/core/accounting/QueryMonitorConfig.java](https://app.codecov.io/gh/apache/pinot/pull/18102?src=pr&el=tree&filepath=pinot-core%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fcore%2Faccounting%2FQueryMonitorConfig.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9hY2NvdW50aW5nL1F1ZXJ5TW9uaXRvckNvbmZpZy5qYXZh) | 66.66% | [10 Missing and 4 partials :warning: ](https://app.codecov.io/gh/apache/pinot/pull/18102?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [.../pinot/core/query/killing/QueryKillingManager.java](https://app.codecov.io/gh/apache/pinot/pull/18102?src=pr&el=tree&filepath=pinot-core%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fcore%2Fquery%2Fkilling%2FQueryKillingManager.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9xdWVyeS9raWxsaW5nL1F1ZXJ5S2lsbGluZ01hbmFnZXIuamF2YQ==) | 75.43% | [11 Missing and 3 partials :warning: ](https://app.codecov.io/gh/apache/pinot/pull/18102?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...killing/strategy/ScanEntriesThresholdStrategy.java](https://app.codecov.io/gh/apache/pinot/pull/18102?src=pr&el=tree&filepath=pinot-core%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fcore%2Fquery%2Fkilling%2Fstrategy%2FScanEntriesThresholdStrategy.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9xdWVyeS9raWxsaW5nL3N0cmF0ZWd5L1NjYW5FbnRyaWVzVGhyZXNob2xkU3RyYXRlZ3kuamF2YQ==) | 90.00% | [0 Missing and 4 partials :warning: ](https://app.codecov.io/gh/apache/pinot/pull/18102?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...e/query/killing/CompositeQueryKillingStrategy.java](https://app.codecov.io/gh/apache/pinot/pull/18102?src=pr&el=tree&filepath=pinot-core%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fcore%2Fquery%2Fkilling%2FCompositeQueryKillingStrategy.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9xdWVyeS9raWxsaW5nL0NvbXBvc2l0ZVF1ZXJ5S2lsbGluZ1N0cmF0ZWd5LmphdmE=) | 92.00% | [1 Missing and 1 partial :warning: ](https://app.codecov.io/gh/apache/pinot/pull/18102?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...pinot/core/query/killing/QueryKillingStrategy.java](https://app.codecov.io/gh/apache/pinot/pull/18102?src=pr&el=tree&filepath=pinot-core%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fcore%2Fquery%2Fkilling%2FQueryKillingStrategy.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9xdWVyeS9raWxsaW5nL1F1ZXJ5S2lsbGluZ1N0cmF0ZWd5LmphdmE=) | 66.66% | [1 Missing :warning: ](https://app.codecov.io/gh/apache/pinot/pull/18102?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...org/apache/pinot/spi/config/table/QueryConfig.java](https://app.codecov.io/gh/apache/pinot/pull/18102?src=pr&el=tree&filepath=pinot-spi%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fspi%2Fconfig%2Ftable%2FQueryConfig.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvY29uZmlnL3RhYmxlL1F1ZXJ5Q29uZmlnLmphdmE=) | 90.00% | [0 Missing and 1 partial :warning: ](https://app.codecov.io/gh/apache/pinot/pull/18102?src=pr&el=tree&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 (36cdab4) and HEAD (36a6c2b). Click for more details.
>
> <details><summary>HEAD has 32 uploads less than BASE</summary>
>
>| Flag | BASE (36cdab4) | HEAD (36a6c2b) |
>|------|------|------|
>|java-21|5|1|
>|unittests1|2|1|
>|unittests|4|1|
>|temurin|9|1|
>|java-11|4|0|
>|unittests2|2|0|
>|integration|5|0|
>|integration2|1|0|
>|integration1|2|0|
>|custom-integration1|2|0|
></details>
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #18102 +/- ##
============================================
- Coverage 63.85% 56.11% -7.74%
+ Complexity 1573 778 -795
============================================
Files 3167 2476 -691
Lines 192092 141606 -50486
Branches 29601 22768 -6833
============================================
- Hits 122666 79468 -43198
+ Misses 59774 55337 -4437
+ Partials 9652 6801 -2851
```
| [Flag](https://app.codecov.io/gh/apache/pinot/pull/18102/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/18102/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/18102/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/18102/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/18102/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/18102/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/18102/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `56.11% <84.07%> (-7.69%)` | :arrow_down: |
| [temurin](https://app.codecov.io/gh/apache/pinot/pull/18102/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `56.11% <84.07%> (-7.74%)` | :arrow_down: |
| [unittests](https://app.codecov.io/gh/apache/pinot/pull/18102/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `56.11% <84.07%> (-7.74%)` | :arrow_down: |
| [unittests1](https://app.codecov.io/gh/apache/pinot/pull/18102/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `56.11% <84.07%> (+0.05%)` | :arrow_up: |
| [unittests2](https://app.codecov.io/gh/apache/pinot/pull/18102/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
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/18102?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] [pinot-server/ proactive-query-killing] (1/2) add initial SPI implementation for supporting query killing based on Scan Cost [pinot]
Posted by "anuragrai16 (via GitHub)" <gi...@apache.org>.
anuragrai16 commented on code in PR #18102:
URL: https://github.com/apache/pinot/pull/18102#discussion_r3125798434
##########
pinot-core/src/main/java/org/apache/pinot/core/query/killing/QueryKillingManager.java:
##########
@@ -0,0 +1,184 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.core.query.killing;
+
+import java.util.concurrent.atomic.AtomicReference;
+import javax.annotation.Nullable;
+import org.apache.pinot.common.metrics.ServerMeter;
+import org.apache.pinot.common.metrics.ServerMetrics;
+import org.apache.pinot.core.accounting.QueryMonitorConfig;
+import org.apache.pinot.core.query.killing.strategy.ScanEntriesThresholdStrategy;
+import org.apache.pinot.spi.config.table.QueryConfig;
+import org.apache.pinot.spi.query.QueryExecutionContext;
+import org.apache.pinot.spi.query.QueryScanCostContext;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+
+/**
+ * Central manager for scan-based query killing. Owns the guard rails and delegates the
+ * actual kill decision to a {@link QueryKillingStrategy}.
+ *
+ * The default factory is {@link ScanEntriesThresholdStrategy.Factory}, which reads
+ * scan thresholds from {@link QueryMonitorConfig}. Custom factories can be configured
+ * via {@code accounting.scan.based.killing.strategy.factory.class.name}.
+ *
+ */
+public class QueryKillingManager {
+ private static final Logger LOGGER = LoggerFactory.getLogger(QueryKillingManager.class);
+
+ private static volatile QueryKillingManager _instance;
Review Comment:
The consumer for this (to be added in next diff) is BaseOperator.checkTermination(), a method called on every nextBlock() invocation across many operator subclasses. Threading the manager through constructors would mean modifying the constructor of every operator, much larger change to review.
2. The manager is also server-scoped, we want to create one instance per JVM, initialized once at startup (BaseServerStarter), and then use it everywhere.
--
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] [pinot-server/ proactive-query-killing] (1/2) add initial SPI implementation for supporting query killing based on Scan Cost [pinot]
Posted by "anuragrai16 (via GitHub)" <gi...@apache.org>.
anuragrai16 commented on code in PR #18102:
URL: https://github.com/apache/pinot/pull/18102#discussion_r3125490926
##########
pinot-core/src/main/java/org/apache/pinot/core/accounting/QueryMonitorConfig.java:
##########
Review Comment:
No, this flag doesn't make sense tbh. I always publish metrics when the feature is enabled, and the metric differentiates between logOnly (QUERY_KILLED_DRY_RUN) and enforce (QUERY_KILLED). Not sure why we would want to disable the metrics when the feature is enabled and seems like a redundant extra config.
--
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] [pinot-server/ proactive-query-killing] (1/2) add initial SPI implementation for supporting query killing based on Scan Cost [pinot]
Posted by "anuragrai16 (via GitHub)" <gi...@apache.org>.
anuragrai16 commented on code in PR #18102:
URL: https://github.com/apache/pinot/pull/18102#discussion_r3125502630
##########
pinot-core/src/main/java/org/apache/pinot/core/query/killing/QueryKillingManager.java:
##########
@@ -0,0 +1,184 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.core.query.killing;
+
+import java.util.concurrent.atomic.AtomicReference;
+import javax.annotation.Nullable;
+import org.apache.pinot.common.metrics.ServerMeter;
+import org.apache.pinot.common.metrics.ServerMetrics;
+import org.apache.pinot.core.accounting.QueryMonitorConfig;
+import org.apache.pinot.core.query.killing.strategy.ScanEntriesThresholdStrategy;
+import org.apache.pinot.spi.config.table.QueryConfig;
+import org.apache.pinot.spi.query.QueryExecutionContext;
+import org.apache.pinot.spi.query.QueryScanCostContext;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+
+/**
+ * Central manager for scan-based query killing. Owns the guard rails and delegates the
+ * actual kill decision to a {@link QueryKillingStrategy}.
+ *
+ * The default factory is {@link ScanEntriesThresholdStrategy.Factory}, which reads
+ * scan thresholds from {@link QueryMonitorConfig}. Custom factories can be configured
+ * via {@code accounting.scan.based.killing.strategy.factory.class.name}.
+ *
+ */
+public class QueryKillingManager {
+ private static final Logger LOGGER = LoggerFactory.getLogger(QueryKillingManager.class);
+
+ private static volatile QueryKillingManager _instance;
+
+ private final AtomicReference<QueryMonitorConfig> _configRef;
+ private final ServerMetrics _serverMetrics;
+
+ /**
+ * Null if: killing is disabled, config is insufficient, or factory failed to load.
+ * Rebuilt when config changes via {@link #rebuildStrategy()}.
+ */
+ @Nullable
+ private volatile QueryKillingStrategy _strategy;
+
+ public QueryKillingManager(AtomicReference<QueryMonitorConfig> configRef, ServerMetrics serverMetrics) {
+ _configRef = configRef;
+ _serverMetrics = serverMetrics;
+ }
+
+ /**
+ * Initializes the singleton instance and builds the strategy from config.
+ * Called once during server startup.
+ */
+ public static void init(AtomicReference<QueryMonitorConfig> configRef, ServerMetrics serverMetrics) {
+ QueryKillingManager manager = new QueryKillingManager(configRef, serverMetrics);
+ manager.rebuildStrategy();
+ _instance = manager;
+ }
+
+ @Nullable
+ public static QueryKillingManager getInstance() {
+ return _instance;
+ }
+
+ /**
+ * Rebuilds the strategy from the current config. Called at init and when
+ * cluster config changes (via the same onChange path that rebuilds QueryMonitorConfig).
+ */
+ public void rebuildStrategy() {
+ QueryMonitorConfig config = _configRef.get();
+ if (config == null || !config.isScanBasedKillingEnabled()) {
+ _strategy = null;
+ return;
+ }
+
+ try {
+ QueryKillingStrategyFactory factory = loadFactory(config);
+ _strategy = factory.create(config);
+ if (_strategy == null) {
+ LOGGER.warn("Scan-based killing is enabled but strategy factory '{}' returned null — "
+ + "required configuration may be missing. Scan-based killing will be effectively disabled.",
+ factory.getName());
+ }
+ } catch (Exception e) {
+ LOGGER.error("Failed to initialize scan-based killing strategy. "
Review Comment:
Hmm, this metric will only emit one time and not really a timeseries, dont see a reason for a metric here. The log should be enough to let me know that the strategy failed to initialize, and we will continue with query killing being a no-op
--
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] [pinot-server/ proactive-query-killing] (1/2) add initial SPI implementation for supporting query killing based on Scan Cost [pinot]
Posted by "shauryachats (via GitHub)" <gi...@apache.org>.
shauryachats commented on code in PR #18102:
URL: https://github.com/apache/pinot/pull/18102#discussion_r3114755612
##########
pinot-core/src/main/java/org/apache/pinot/core/accounting/QueryMonitorConfig.java:
##########
@@ -280,6 +316,72 @@ public QueryMonitorConfig(PinotConfiguration config, long maxHeapSize) {
} else {
_workloadCostEnforcementEnabled = oldConfig._workloadCostEnforcementEnabled;
}
+
+ if (changedConfigs.contains(CommonConstants.Accounting.CONFIG_OF_SCAN_BASED_KILLING_MODE)) {
+ _scanBasedKillingMode = validateScanKillingMode(clusterConfigs.getOrDefault(
+ CommonConstants.Accounting.CONFIG_OF_SCAN_BASED_KILLING_MODE,
+ CommonConstants.Accounting.DEFAULT_SCAN_BASED_KILLING_MODE));
+ } else {
+ _scanBasedKillingMode = oldConfig.getScanBasedKillingMode();
+ }
+
+ if (changedConfigs.contains(
+ CommonConstants.Accounting.CONFIG_OF_SCAN_BASED_KILLING_MAX_ENTRIES_SCANNED_IN_FILTER)) {
+ _scanBasedKillingMaxEntriesScannedInFilter = Long.parseLong(
+ clusterConfigs.getOrDefault(
+ CommonConstants.Accounting.CONFIG_OF_SCAN_BASED_KILLING_MAX_ENTRIES_SCANNED_IN_FILTER,
+ String.valueOf(
+ CommonConstants.Accounting.DEFAULT_SCAN_BASED_KILLING_MAX_ENTRIES_SCANNED_IN_FILTER)));
+ } else {
+ _scanBasedKillingMaxEntriesScannedInFilter =
+ oldConfig.getScanBasedKillingMaxEntriesScannedInFilter();
+ }
+
+ if (changedConfigs.contains(
+ CommonConstants.Accounting.CONFIG_OF_SCAN_BASED_KILLING_MAX_DOCS_SCANNED)) {
+ _scanBasedKillingMaxDocsScanned = Long.parseLong(
+ clusterConfigs.getOrDefault(
+ CommonConstants.Accounting.CONFIG_OF_SCAN_BASED_KILLING_MAX_DOCS_SCANNED,
+ String.valueOf(CommonConstants.Accounting.DEFAULT_SCAN_BASED_KILLING_MAX_DOCS_SCANNED)));
+ } else {
+ _scanBasedKillingMaxDocsScanned = oldConfig.getScanBasedKillingMaxDocsScanned();
+ }
+
+ if (changedConfigs.contains(
+ CommonConstants.Accounting.CONFIG_OF_SCAN_BASED_KILLING_MAX_ENTRIES_SCANNED_POST_FILTER)) {
+ _scanBasedKillingMaxEntriesScannedPostFilter = Long.parseLong(
Review Comment:
Guard against invalid string values (empty string for example).
--
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] [pinot-server/ proactive-query-killing] (1/2) add initial SPI implementation for supporting query killing based on Scan Cost [pinot]
Posted by "tarun11Mavani (via GitHub)" <gi...@apache.org>.
tarun11Mavani commented on code in PR #18102:
URL: https://github.com/apache/pinot/pull/18102#discussion_r3108405204
##########
pinot-core/src/main/java/org/apache/pinot/core/query/killing/QueryKillingManager.java:
##########
@@ -0,0 +1,184 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.core.query.killing;
+
+import java.util.concurrent.atomic.AtomicReference;
+import javax.annotation.Nullable;
+import org.apache.pinot.common.metrics.ServerMeter;
+import org.apache.pinot.common.metrics.ServerMetrics;
+import org.apache.pinot.core.accounting.QueryMonitorConfig;
+import org.apache.pinot.core.query.killing.strategy.ScanEntriesThresholdStrategy;
+import org.apache.pinot.spi.config.table.QueryConfig;
+import org.apache.pinot.spi.query.QueryExecutionContext;
+import org.apache.pinot.spi.query.QueryScanCostContext;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+
+/**
+ * Central manager for scan-based query killing. Owns the guard rails and delegates the
+ * actual kill decision to a {@link QueryKillingStrategy}.
+ *
+ * The default factory is {@link ScanEntriesThresholdStrategy.Factory}, which reads
+ * scan thresholds from {@link QueryMonitorConfig}. Custom factories can be configured
+ * via {@code accounting.scan.based.killing.strategy.factory.class.name}.
+ *
+ */
+public class QueryKillingManager {
+ private static final Logger LOGGER = LoggerFactory.getLogger(QueryKillingManager.class);
+
+ private static volatile QueryKillingManager _instance;
+
+ private final AtomicReference<QueryMonitorConfig> _configRef;
+ private final ServerMetrics _serverMetrics;
+
+ /**
+ * Null if: killing is disabled, config is insufficient, or factory failed to load.
+ * Rebuilt when config changes via {@link #rebuildStrategy()}.
+ */
+ @Nullable
+ private volatile QueryKillingStrategy _strategy;
+
+ public QueryKillingManager(AtomicReference<QueryMonitorConfig> configRef, ServerMetrics serverMetrics) {
+ _configRef = configRef;
+ _serverMetrics = serverMetrics;
+ }
+
+ /**
+ * Initializes the singleton instance and builds the strategy from config.
+ * Called once during server startup.
+ */
+ public static void init(AtomicReference<QueryMonitorConfig> configRef, ServerMetrics serverMetrics) {
+ QueryKillingManager manager = new QueryKillingManager(configRef, serverMetrics);
+ manager.rebuildStrategy();
+ _instance = manager;
+ }
+
+ @Nullable
+ public static QueryKillingManager getInstance() {
+ return _instance;
+ }
+
+ /**
+ * Rebuilds the strategy from the current config. Called at init and when
+ * cluster config changes (via the same onChange path that rebuilds QueryMonitorConfig).
+ */
+ public void rebuildStrategy() {
+ QueryMonitorConfig config = _configRef.get();
+ if (config == null || !config.isScanBasedKillingEnabled()) {
+ _strategy = null;
+ return;
+ }
+
+ try {
+ QueryKillingStrategyFactory factory = loadFactory(config);
+ _strategy = factory.create(config);
+ if (_strategy == null) {
+ LOGGER.warn("Scan-based killing is enabled but strategy factory '{}' returned null — "
+ + "required configuration may be missing. Scan-based killing will be effectively disabled.",
+ factory.getName());
+ }
+ } catch (Exception e) {
+ LOGGER.error("Failed to initialize scan-based killing strategy. "
+ + "Scan-based killing will be disabled.", e);
+ _strategy = null;
+ }
+ }
+
+ /**
+ * Loads the strategy factory from config. If a custom factory class name is configured,
+ * loads it by reflection (following the same pattern as {@code ThreadAccountantUtils.createAccountant()}).
+ * Otherwise, returns the default {@link ScanEntriesThresholdStrategy.Factory}.
+ */
+ private QueryKillingStrategyFactory loadFactory(QueryMonitorConfig config) {
+ String factoryClassName = config.getScanBasedKillingStrategyFactoryClassName();
+ if (factoryClassName != null && !factoryClassName.isEmpty()) {
+ LOGGER.info("Loading custom query killing strategy factory: {}", factoryClassName);
+ try {
+ return (QueryKillingStrategyFactory) Class.forName(factoryClassName)
+ .getDeclaredConstructor().newInstance();
+ } catch (Exception e) {
+ LOGGER.error("Failed to load custom strategy factory '{}', falling back to default",
+ factoryClassName, e);
+ }
+ }
+ return new ScanEntriesThresholdStrategy.Factory();
+ }
+
+ /**
+ * Returns the active strategy. Visible for testing.
+ */
+ @Nullable
+ public QueryKillingStrategy getActiveStrategy() {
+ return _strategy;
+ }
+
+ /**
+ * Evaluates whether the query should be killed based on the active strategy.
+ *
+ * <p>Calls {@link QueryKillingStrategy#forQuery(QueryConfig, QueryMonitorConfig)}
+ * to resolve table-level overrides before evaluating.</p>
+ */
+ public void checkAndKillIfNeeded(QueryExecutionContext executionContext,
+ QueryScanCostContext scanCostContext, String queryId, String tableName,
+ @Nullable QueryConfig queryConfig) {
+ // no strategy means killing is disabled or unconfigured
+ QueryKillingStrategy strategy = _strategy;
+ if (strategy == null) {
+ return;
+ }
+
+ QueryMonitorConfig config = _configRef.get();
+ if (config == null || !config.isScanBasedKillingEnabled()) {
+ return;
+ }
+
+ // Prevent duplicate kills
+ if (executionContext.getTerminateException() != null) {
+ return;
+ }
+
+ try {
+ // Resolve per-query table overrides (returns same instance if no overrides)
+ QueryKillingStrategy queryStrategy = strategy.forQuery(queryConfig, config);
+
+ String configSource = (queryStrategy != strategy) ? "table:" + tableName : "cluster";
+
+ // Delegate to strategy
+ if (!queryStrategy.shouldTerminate(scanCostContext)) {
+ return;
+ }
+
+ QueryKillReport report = queryStrategy.buildKillReport(
+ scanCostContext, queryId, tableName, configSource);
+
+ if (config.isScanBasedKillingLogOnly()) {
+ LOGGER.info("Query killed in LogOnly mode: {}", report.toInternalLogMessage());
+ _serverMetrics.addMeteredGlobalValue(ServerMeter.QUERIES_KILLED_SCAN_DRY_RUN, 1);
+ return;
+ }
+
+ LOGGER.warn("Query Killed in enforce mode: {}", report.toInternalLogMessage());
+ executionContext.terminate(queryStrategy.getErrorCode(), report.toCustomerMessage());
+ _serverMetrics.addMeteredGlobalValue(ServerMeter.QUERIES_KILLED_SCAN, 1);
+ } catch (Exception e) {
+ LOGGER.error("Error in scan-based killing evaluation for query {}", queryId, e);
Review Comment:
:nit: We can add a metric here.
##########
pinot-core/src/main/java/org/apache/pinot/core/accounting/QueryMonitorConfig.java:
##########
Review Comment:
Side q: Are we honoring this flag during emitting the metics in future PRs? worth a look.
##########
pinot-core/src/main/java/org/apache/pinot/core/accounting/QueryMonitorConfig.java:
##########
@@ -20,11 +20,21 @@
import java.util.Map;
import java.util.Set;
+import javax.annotation.Nullable;
import org.apache.pinot.spi.env.PinotConfiguration;
import org.apache.pinot.spi.utils.CommonConstants;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
public class QueryMonitorConfig {
+ private static final Logger LOGGER = LoggerFactory.getLogger(QueryMonitorConfig.class);
+
+ private static final Set<String> VALID_SCAN_KILLING_MODES = Set.of(
Review Comment:
Can we replace this and `_scanBasedKillingMode` with `public enum ScanKillingMode { DISABLED, LOG_ONLY, ENFORCE }`
##########
pinot-core/src/main/java/org/apache/pinot/core/accounting/QueryMonitorConfig.java:
##########
@@ -280,6 +316,72 @@ public QueryMonitorConfig(PinotConfiguration config, long maxHeapSize) {
} else {
_workloadCostEnforcementEnabled = oldConfig._workloadCostEnforcementEnabled;
}
+
+ if (changedConfigs.contains(CommonConstants.Accounting.CONFIG_OF_SCAN_BASED_KILLING_MODE)) {
+ _scanBasedKillingMode = validateScanKillingMode(clusterConfigs.getOrDefault(
+ CommonConstants.Accounting.CONFIG_OF_SCAN_BASED_KILLING_MODE,
+ CommonConstants.Accounting.DEFAULT_SCAN_BASED_KILLING_MODE));
+ } else {
+ _scanBasedKillingMode = oldConfig.getScanBasedKillingMode();
+ }
+
+ if (changedConfigs.contains(
+ CommonConstants.Accounting.CONFIG_OF_SCAN_BASED_KILLING_MAX_ENTRIES_SCANNED_IN_FILTER)) {
+ _scanBasedKillingMaxEntriesScannedInFilter = Long.parseLong(
+ clusterConfigs.getOrDefault(
+ CommonConstants.Accounting.CONFIG_OF_SCAN_BASED_KILLING_MAX_ENTRIES_SCANNED_IN_FILTER,
+ String.valueOf(
+ CommonConstants.Accounting.DEFAULT_SCAN_BASED_KILLING_MAX_ENTRIES_SCANNED_IN_FILTER)));
+ } else {
+ _scanBasedKillingMaxEntriesScannedInFilter =
+ oldConfig.getScanBasedKillingMaxEntriesScannedInFilter();
+ }
+
+ if (changedConfigs.contains(
+ CommonConstants.Accounting.CONFIG_OF_SCAN_BASED_KILLING_MAX_DOCS_SCANNED)) {
+ _scanBasedKillingMaxDocsScanned = Long.parseLong(
+ clusterConfigs.getOrDefault(
+ CommonConstants.Accounting.CONFIG_OF_SCAN_BASED_KILLING_MAX_DOCS_SCANNED,
+ String.valueOf(CommonConstants.Accounting.DEFAULT_SCAN_BASED_KILLING_MAX_DOCS_SCANNED)));
+ } else {
+ _scanBasedKillingMaxDocsScanned = oldConfig.getScanBasedKillingMaxDocsScanned();
+ }
+
+ if (changedConfigs.contains(
+ CommonConstants.Accounting.CONFIG_OF_SCAN_BASED_KILLING_MAX_ENTRIES_SCANNED_POST_FILTER)) {
+ _scanBasedKillingMaxEntriesScannedPostFilter = Long.parseLong(
+ clusterConfigs.getOrDefault(
+ CommonConstants.Accounting.CONFIG_OF_SCAN_BASED_KILLING_MAX_ENTRIES_SCANNED_POST_FILTER,
+ String.valueOf(
+ CommonConstants.Accounting.DEFAULT_SCAN_BASED_KILLING_MAX_ENTRIES_SCANNED_POST_FILTER)));
+ } else {
+ _scanBasedKillingMaxEntriesScannedPostFilter =
+ oldConfig.getScanBasedKillingMaxEntriesScannedPostFilter();
+ }
+
+ if (changedConfigs.contains(
+ CommonConstants.Accounting.CONFIG_OF_SCAN_BASED_KILLING_STRATEGY_FACTORY_CLASS_NAME)) {
+ _scanBasedKillingStrategyFactoryClassName = clusterConfigs.getOrDefault(
+ CommonConstants.Accounting.CONFIG_OF_SCAN_BASED_KILLING_STRATEGY_FACTORY_CLASS_NAME, null);
+ } else {
+ _scanBasedKillingStrategyFactoryClassName = oldConfig.getScanBasedKillingStrategyFactoryClassName();
+ }
+ }
+
+ /**
+ * Validates the scan-based killing mode. If the value is not one of the recognized modes
+ * (disabled, logOnly, enforce), logs an error and falls back to "disabled" so the server
+ * continues to start normally.
+ */
+ private static String validateScanKillingMode(String mode) {
Review Comment:
This can be removed if we use enum.
--
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] [pinot-server/ proactive-query-killing] (1/2) add initial SPI implementation for supporting query killing based on Scan Cost [pinot]
Posted by "shauryachats (via GitHub)" <gi...@apache.org>.
shauryachats merged PR #18102:
URL: https://github.com/apache/pinot/pull/18102
--
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] [pinot-server/ proactive-query-killing] (1/2) add initial SPI implementation for supporting query killing based on Scan Cost [pinot]
Posted by "shauryachats (via GitHub)" <gi...@apache.org>.
shauryachats commented on code in PR #18102:
URL: https://github.com/apache/pinot/pull/18102#discussion_r3114757793
##########
pinot-core/src/main/java/org/apache/pinot/core/query/killing/QueryKillingManager.java:
##########
@@ -0,0 +1,184 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.core.query.killing;
+
+import java.util.concurrent.atomic.AtomicReference;
+import javax.annotation.Nullable;
+import org.apache.pinot.common.metrics.ServerMeter;
+import org.apache.pinot.common.metrics.ServerMetrics;
+import org.apache.pinot.core.accounting.QueryMonitorConfig;
+import org.apache.pinot.core.query.killing.strategy.ScanEntriesThresholdStrategy;
+import org.apache.pinot.spi.config.table.QueryConfig;
+import org.apache.pinot.spi.query.QueryExecutionContext;
+import org.apache.pinot.spi.query.QueryScanCostContext;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+
+/**
+ * Central manager for scan-based query killing. Owns the guard rails and delegates the
+ * actual kill decision to a {@link QueryKillingStrategy}.
+ *
+ * The default factory is {@link ScanEntriesThresholdStrategy.Factory}, which reads
+ * scan thresholds from {@link QueryMonitorConfig}. Custom factories can be configured
+ * via {@code accounting.scan.based.killing.strategy.factory.class.name}.
+ *
+ */
+public class QueryKillingManager {
+ private static final Logger LOGGER = LoggerFactory.getLogger(QueryKillingManager.class);
+
+ private static volatile QueryKillingManager _instance;
+
+ private final AtomicReference<QueryMonitorConfig> _configRef;
+ private final ServerMetrics _serverMetrics;
+
+ /**
+ * Null if: killing is disabled, config is insufficient, or factory failed to load.
+ * Rebuilt when config changes via {@link #rebuildStrategy()}.
+ */
+ @Nullable
+ private volatile QueryKillingStrategy _strategy;
+
+ public QueryKillingManager(AtomicReference<QueryMonitorConfig> configRef, ServerMetrics serverMetrics) {
+ _configRef = configRef;
+ _serverMetrics = serverMetrics;
+ }
+
+ /**
+ * Initializes the singleton instance and builds the strategy from config.
+ * Called once during server startup.
+ */
+ public static void init(AtomicReference<QueryMonitorConfig> configRef, ServerMetrics serverMetrics) {
+ QueryKillingManager manager = new QueryKillingManager(configRef, serverMetrics);
+ manager.rebuildStrategy();
+ _instance = manager;
+ }
+
+ @Nullable
+ public static QueryKillingManager getInstance() {
+ return _instance;
+ }
+
+ /**
+ * Rebuilds the strategy from the current config. Called at init and when
+ * cluster config changes (via the same onChange path that rebuilds QueryMonitorConfig).
+ */
+ public void rebuildStrategy() {
+ QueryMonitorConfig config = _configRef.get();
+ if (config == null || !config.isScanBasedKillingEnabled()) {
+ _strategy = null;
+ return;
+ }
+
+ try {
+ QueryKillingStrategyFactory factory = loadFactory(config);
+ _strategy = factory.create(config);
+ if (_strategy == null) {
+ LOGGER.warn("Scan-based killing is enabled but strategy factory '{}' returned null — "
+ + "required configuration may be missing. Scan-based killing will be effectively disabled.",
+ factory.getName());
+ }
+ } catch (Exception e) {
+ LOGGER.error("Failed to initialize scan-based killing strategy. "
Review Comment:
Can we also add metrics here for production monitoring?
--
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] [pinot-server/ proactive-query-killing] (1/2) add initial SPI implementation for supporting query killing based on Scan Cost [pinot]
Posted by "shauryachats (via GitHub)" <gi...@apache.org>.
shauryachats commented on PR #18102:
URL: https://github.com/apache/pinot/pull/18102#issuecomment-4324342307
@anuragrai16 Pinot Binary Compatibility Check is failing, can you please rebase your PR?
--
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] [pinot-server/ proactive-query-killing] (1/2) add initial SPI implementation for supporting query killing based on Scan Cost [pinot]
Posted by "shauryachats (via GitHub)" <gi...@apache.org>.
shauryachats commented on code in PR #18102:
URL: https://github.com/apache/pinot/pull/18102#discussion_r3114782098
##########
pinot-core/src/main/java/org/apache/pinot/core/query/killing/QueryKillingManager.java:
##########
@@ -0,0 +1,184 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.core.query.killing;
+
+import java.util.concurrent.atomic.AtomicReference;
+import javax.annotation.Nullable;
+import org.apache.pinot.common.metrics.ServerMeter;
+import org.apache.pinot.common.metrics.ServerMetrics;
+import org.apache.pinot.core.accounting.QueryMonitorConfig;
+import org.apache.pinot.core.query.killing.strategy.ScanEntriesThresholdStrategy;
+import org.apache.pinot.spi.config.table.QueryConfig;
+import org.apache.pinot.spi.query.QueryExecutionContext;
+import org.apache.pinot.spi.query.QueryScanCostContext;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+
+/**
+ * Central manager for scan-based query killing. Owns the guard rails and delegates the
+ * actual kill decision to a {@link QueryKillingStrategy}.
+ *
+ * The default factory is {@link ScanEntriesThresholdStrategy.Factory}, which reads
+ * scan thresholds from {@link QueryMonitorConfig}. Custom factories can be configured
+ * via {@code accounting.scan.based.killing.strategy.factory.class.name}.
+ *
+ */
+public class QueryKillingManager {
+ private static final Logger LOGGER = LoggerFactory.getLogger(QueryKillingManager.class);
+
+ private static volatile QueryKillingManager _instance;
Review Comment:
Any particular reason we are keeping a `static volatile` variable and not passing it to relevant classes through DI?
--
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] [pinot-server/ proactive-query-killing] (1/2) add initial SPI implementation for supporting query killing based on Scan Cost [pinot]
Posted by "anuragrai16 (via GitHub)" <gi...@apache.org>.
anuragrai16 commented on code in PR #18102:
URL: https://github.com/apache/pinot/pull/18102#discussion_r3125839373
##########
pinot-core/src/main/java/org/apache/pinot/core/accounting/QueryMonitorConfig.java:
##########
@@ -280,6 +316,72 @@ public QueryMonitorConfig(PinotConfiguration config, long maxHeapSize) {
} else {
_workloadCostEnforcementEnabled = oldConfig._workloadCostEnforcementEnabled;
}
+
+ if (changedConfigs.contains(CommonConstants.Accounting.CONFIG_OF_SCAN_BASED_KILLING_MODE)) {
+ _scanBasedKillingMode = validateScanKillingMode(clusterConfigs.getOrDefault(
+ CommonConstants.Accounting.CONFIG_OF_SCAN_BASED_KILLING_MODE,
+ CommonConstants.Accounting.DEFAULT_SCAN_BASED_KILLING_MODE));
+ } else {
+ _scanBasedKillingMode = oldConfig.getScanBasedKillingMode();
+ }
+
+ if (changedConfigs.contains(
+ CommonConstants.Accounting.CONFIG_OF_SCAN_BASED_KILLING_MAX_ENTRIES_SCANNED_IN_FILTER)) {
+ _scanBasedKillingMaxEntriesScannedInFilter = Long.parseLong(
+ clusterConfigs.getOrDefault(
+ CommonConstants.Accounting.CONFIG_OF_SCAN_BASED_KILLING_MAX_ENTRIES_SCANNED_IN_FILTER,
+ String.valueOf(
+ CommonConstants.Accounting.DEFAULT_SCAN_BASED_KILLING_MAX_ENTRIES_SCANNED_IN_FILTER)));
+ } else {
+ _scanBasedKillingMaxEntriesScannedInFilter =
+ oldConfig.getScanBasedKillingMaxEntriesScannedInFilter();
+ }
+
+ if (changedConfigs.contains(
+ CommonConstants.Accounting.CONFIG_OF_SCAN_BASED_KILLING_MAX_DOCS_SCANNED)) {
+ _scanBasedKillingMaxDocsScanned = Long.parseLong(
+ clusterConfigs.getOrDefault(
+ CommonConstants.Accounting.CONFIG_OF_SCAN_BASED_KILLING_MAX_DOCS_SCANNED,
+ String.valueOf(CommonConstants.Accounting.DEFAULT_SCAN_BASED_KILLING_MAX_DOCS_SCANNED)));
+ } else {
+ _scanBasedKillingMaxDocsScanned = oldConfig.getScanBasedKillingMaxDocsScanned();
+ }
+
+ if (changedConfigs.contains(
+ CommonConstants.Accounting.CONFIG_OF_SCAN_BASED_KILLING_MAX_ENTRIES_SCANNED_POST_FILTER)) {
+ _scanBasedKillingMaxEntriesScannedPostFilter = Long.parseLong(
Review Comment:
thanks, handled
--
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