Queries now self terminate if in panic mode. by vrajat · Pull Request #16380 · apache/pinot

vrajat · 2025-07-18T07:40:43Z

Queries will pro-actively terminate if heap usage is in panic mode in the ThreadResourceUsageAccountant.

Note that #16040 is required for this feature to work correctly.

To support this feature, the thread accountant with the unmentionable name adds isQueryTerminated()
This function is also checked in Tracing.ThreadAccountantOps.isInterrupted

gortiz

LGTM

gortiz · 2025-07-22T07:08:10Z

pinot-core/src/main/java/org/apache/pinot/core/accounting/PerQueryCPUMemAccountantFactory.java


    protected Set<String> _cancelSentQueries;

+    protected String _maxMemoryUsageQueryId;


nit: rememer to mark it as @Nullable

BTW, is this attribute modified in a single thread fashion?

Yes. It is only updated by the watcher task.

Changed to atomic reference

codecov-commenter · 2025-07-22T13:32:53Z

Codecov Report

Attention: Patch coverage is 42.85714% with 12 lines in your changes missing coverage. Please review.

Project coverage is 63.19%. Comparing base (1a476de) to head (429d0c9).
Report is 512 commits behind head on master.

Files with missing lines	Patch %	Lines
...re/accounting/PerQueryCPUMemAccountantFactory.java	20.00%	7 Missing and 1 partial ⚠️
...ache/pinot/core/accounting/QueryMonitorConfig.java	75.00%	1 Missing and 1 partial ⚠️
.../main/java/org/apache/pinot/spi/trace/Tracing.java	0.00%	0 Missing and 2 partials ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##             master   #16380      +/-   ##
============================================
+ Coverage     62.90%   63.19%   +0.29%     
+ Complexity     1386     1364      -22     
============================================
  Files          2867     3006     +139     
  Lines        163354   174055   +10701     
  Branches      24952    26621    +1669     
============================================
+ Hits         102755   110002    +7247     
- Misses        52847    55662    +2815     
- Partials       7752     8391     +639

Flag	Coverage Δ
custom-integration1	`100.00% <ø> (ø)`
integration	`100.00% <ø> (ø)`
integration1	`100.00% <ø> (ø)`
integration2	`0.00% <ø> (ø)`
java-11	`63.16% <42.85%> (+0.29%)`	⬆️
java-21	`63.17% <42.85%> (+0.35%)`	⬆️
skip-bytebuffers-false	`?`
skip-bytebuffers-true	`?`
temurin	`63.19% <42.85%> (+0.29%)`	⬆️
unittests	`63.19% <42.85%> (+0.29%)`	⬆️
unittests1	`56.31% <42.85%> (+0.49%)`	⬆️
unittests2	`33.18% <0.00%> (-0.39%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

xiangfu0 · 2025-07-23T08:25:48Z

pinot-core/src/main/java/org/apache/pinot/core/accounting/PerQueryCPUMemAccountantFactory.java

          if (_triggeringLevel.ordinal() > TriggeringLevel.Normal.ordinal()) {
            _aggregatedUsagePerActiveQuery = getQueryResourcesImpl();
+            _aggregatedUsagePerActiveQuery.values().stream()
+                .filter(stats -> !_cancelSentQueries.contains(stats.getQueryId()))


we should kill all the queries associated with a global id.
Also the accounting should be grouped all queries together

Right. I have changed the filter to only choose queries if greater than min. footprint.

pinot-core/src/main/java/org/apache/pinot/core/accounting/PerQueryCPUMemAccountantFactory.java

xiangfu0 · 2025-07-24T07:20:12Z

Can we split this to panic level and critical level separately.
Panic level handling is fine.
I think the critical level handling for the query id still needs more polish

vrajat · 2025-07-24T13:11:55Z

Can we split this to panic level and critical level separately. Panic level handling is fine. I think the critical level handling for the query id still needs more polish

I've removed the code for critical mode for now.

* [Query Resource Isolation] Workload Configs (apache#15109) * Workload Configs * workload config * Add API * config * Change config structure * Propagation strategy * Fix style check * Cost spliting on update * Table addition propagation * perf * Tests * test * test 2 * Review comments 1 * review comments 3 * review comments 3 * name change * review comments 4 * Fix TableDoesNotExistError for hybrid tables in MSE queries in controller API (apache#16102) * Make ThreadResourceUsageProvider a Helper/Utility Class. (apache#16051) * ThreadResourceUsageProvider is a helper class. ThreadResourceContext tracks resource usage. Fix updateConcurrently * Rename to ThreadResourceSnapshot * Clean up * Add javadoc * Done use auto closeable * Checkstyle * Fix compilation error * Add back removed functions in SPI * Remove private constructor because japicmp complains. * Add setThreadResourceUsageProvider because of backward-incompatible checks * Add setThreadResourceUsageProvider because of backward-incompatible checks * Fix test * Fix ThreadResourceSnapshot usage and tests * Store cpu sample in nanoseconds. * Reduce logs and improve logging when queries are terminated due to OOM. (apache#16172) * Dynamic PerQueryCPUMemAccountant Config on Servers (apache#16219) * Checkpoint * Register change handler * Fix bugs. Manually tested * Checkstyle * Tests * Add pre-check that values are default * Undo typo fix * Update QueryRunner to make use of window function overflow handling server configurations (apache#16108) * Add multistage thread limiting configs at the broker and server level (apache#16080) * Adding changes for supporting RLS (apache#16043) * Use stats cache on error instead of the chained mechanism (apache#15992) * Improve broker error messaging when broker is the one reporting the failure (apache#16076) * Introduce MSE active and passive timeouts (apache#16075) * Throttle SSE & MSE Tasks if Server heap usage is above a threshold (apache#16271) * Fix QueryScheduler constructor using class name. (apache#16280) * Fix QueryScheduler constructor using class name. * Fix test * [Query Resource Isolation] WorkloadBudgetManager and Host enforcement (apache#15798) * QRI - WorkloadBudgetManager implementation * Address review comments * Remove singleton & signature fix * Fix compatibility checker * Review comments * Move WorkloadBudgetManager to core. --------- Co-authored-by: praveenc7 <praveenkchaganlal@gmail.com> * Eliminate duplicate cancel attempts in PerQueryCPUMemAccountant (apache#16299) * Add basic 1 query tests * Add more tests * Add ability to remember cancel queries. * Clean up if conditions in killMostExpensiveQuery * Fix test failures. * Address review comments. * Use QueryCancelCallback to cancel queries from ThreadResourceUsageAccountant (apache#16142) * Remove all calls to System.gc() in PerQueryCPUMemAccountantFactory (apache#16374) * Initialize thread accountant just before serving queries (apache#16326) * Allow Reset of ThreadResourceUsageAccountant in Tracing.java (apache#16360) * Queries now self terminate if in panic mode. (apache#16380) * Queries now self terminate if in panic mode. * Add config test * Hard kill on critical level. * Fix configs * Separate anchor thread interruption. * Checkstyle * Review comments * remove code for critical level --------- Co-authored-by: Rajat Venkatesh <vrajat@users.noreply.github.com> * [Query Resource Isolation] Additonal Sampling for Broker and Server (apache#16164) * fix * sampling * Broker sampling * revert integ-test * Fix test failures * review comments * remove MSE * broker auth * remove per pruner & planner sample * Use Broker's accountant to sample in the request handler. (apache#16439) * [Query Resource Isolation] Workload Scheduler (apache#16018) * QRI - WorkloadBudgetManager implementation * Address review comments * scheduler * unit test * review comments: metrics, secondary, resource-manager * remove broker admission * Remove default budget --------- Co-authored-by: Vivek Iyer Vaidyanathan Iyer <vvaidyanathan@linkedin.com> * Cleanup deprecated methods in ThreadResourceUsageAccountant (apache#16479) * Remove unnecessary methods and config for ThreadResourceUsageAccountant (apache#16490) * Add tests for OOM Termination of MSE queries. (apache#16514) * Fix a flaky assert when testing OOM Cancellation of MSE Queries (apache#16533) * Disable Flaky Tests (apache#16554) This is a follow-up to apache#16533 The fix for a flaky test did not work. This PR disables these tests temporarily. * Use correlation ID instead of request id in PerQueryCpuMemAccountant (apache#16040) * [Query Resource Isolation]Interface for Workload Stats Collection (apache#16340) * Interface for Stats Collection * Additional comments * inherit * additional class comments * [Query Resource Isolation] Fix Refresh message (apache#16636) * Fix Refresh message * delete queryworkload message handler * info -> debug logs * reduce logging (apache#16698) * style check * [Query Workload Isolation] Cost-split support (apache#16672) * splits * Cost split * test * propagation entity change & java doc * Propagation scheme review comments * empty commit to trigger build * Reduce log for PerQueryCPUMemResourceUsageAccountant (apache#16642) --------- Co-authored-by: Rajat Venkatesh <1638298+vrajat@users.noreply.github.com> Co-authored-by: Yash Mayya <yash.mayya@gmail.com> Co-authored-by: Satwik Pachigolla <40644097+satwik-pachigolla@users.noreply.github.com> Co-authored-by: 9aman <35227405+9aman@users.noreply.github.com> Co-authored-by: Gonzalo Ortiz Jaureguizar <gortiz@users.noreply.github.com> Co-authored-by: Vivek Iyer Vaidyanathan <vvivekiyer@gmail.com> Co-authored-by: Xiaotian (Jackie) Jiang <17555551+Jackie-Jiang@users.noreply.github.com> Co-authored-by: Rajat Venkatesh <vrajat@users.noreply.github.com> Co-authored-by: Vivek Iyer Vaidyanathan Iyer <vvaidyanathan@linkedin.com>

* [Query Resource Isolation] Workload Configs (apache#15109) * Workload Configs * workload config * Add API * config * Change config structure * Propagation strategy * Fix style check * Cost spliting on update * Table addition propagation * perf * Tests * test * test 2 * Review comments 1 * review comments 3 * review comments 3 * name change * review comments 4 * Fix TableDoesNotExistError for hybrid tables in MSE queries in controller API (apache#16102) * Make ThreadResourceUsageProvider a Helper/Utility Class. (apache#16051) * ThreadResourceUsageProvider is a helper class. ThreadResourceContext tracks resource usage. Fix updateConcurrently * Rename to ThreadResourceSnapshot * Clean up * Add javadoc * Done use auto closeable * Checkstyle * Fix compilation error * Add back removed functions in SPI * Remove private constructor because japicmp complains. * Add setThreadResourceUsageProvider because of backward-incompatible checks * Add setThreadResourceUsageProvider because of backward-incompatible checks * Fix test * Fix ThreadResourceSnapshot usage and tests * Store cpu sample in nanoseconds. * Reduce logs and improve logging when queries are terminated due to OOM. (apache#16172) * Dynamic PerQueryCPUMemAccountant Config on Servers (apache#16219) * Checkpoint * Register change handler * Fix bugs. Manually tested * Checkstyle * Tests * Add pre-check that values are default * Undo typo fix * Update QueryRunner to make use of window function overflow handling server configurations (apache#16108) * Add multistage thread limiting configs at the broker and server level (apache#16080) * Adding changes for supporting RLS (apache#16043) * Use stats cache on error instead of the chained mechanism (apache#15992) * Improve broker error messaging when broker is the one reporting the failure (apache#16076) * Introduce MSE active and passive timeouts (apache#16075) * Throttle SSE & MSE Tasks if Server heap usage is above a threshold (apache#16271) * Fix QueryScheduler constructor using class name. (apache#16280) * Fix QueryScheduler constructor using class name. * Fix test * [Query Resource Isolation] WorkloadBudgetManager and Host enforcement (apache#15798) * QRI - WorkloadBudgetManager implementation * Address review comments * Remove singleton & signature fix * Fix compatibility checker * Review comments * Move WorkloadBudgetManager to core. --------- Co-authored-by: praveenc7 <praveenkchaganlal@gmail.com> * Eliminate duplicate cancel attempts in PerQueryCPUMemAccountant (apache#16299) * Add basic 1 query tests * Add more tests * Add ability to remember cancel queries. * Clean up if conditions in killMostExpensiveQuery * Fix test failures. * Address review comments. * Use QueryCancelCallback to cancel queries from ThreadResourceUsageAccountant (apache#16142) * Remove all calls to System.gc() in PerQueryCPUMemAccountantFactory (apache#16374) * Initialize thread accountant just before serving queries (apache#16326) * Allow Reset of ThreadResourceUsageAccountant in Tracing.java (apache#16360) * Queries now self terminate if in panic mode. (apache#16380) * Queries now self terminate if in panic mode. * Add config test * Hard kill on critical level. * Fix configs * Separate anchor thread interruption. * Checkstyle * Review comments * remove code for critical level --------- Co-authored-by: Rajat Venkatesh <vrajat@users.noreply.github.com> * [Query Resource Isolation] Additonal Sampling for Broker and Server (apache#16164) * fix * sampling * Broker sampling * revert integ-test * Fix test failures * review comments * remove MSE * broker auth * remove per pruner & planner sample * Use Broker's accountant to sample in the request handler. (apache#16439) * [Query Resource Isolation] Workload Scheduler (apache#16018) * QRI - WorkloadBudgetManager implementation * Address review comments * scheduler * unit test * review comments: metrics, secondary, resource-manager * remove broker admission * Remove default budget --------- Co-authored-by: Vivek Iyer Vaidyanathan Iyer <vvaidyanathan@linkedin.com> * Cleanup deprecated methods in ThreadResourceUsageAccountant (apache#16479) * Remove unnecessary methods and config for ThreadResourceUsageAccountant (apache#16490) * Add tests for OOM Termination of MSE queries. (apache#16514) * Fix a flaky assert when testing OOM Cancellation of MSE Queries (apache#16533) * Disable Flaky Tests (apache#16554) This is a follow-up to apache#16533 The fix for a flaky test did not work. This PR disables these tests temporarily. * Use correlation ID instead of request id in PerQueryCpuMemAccountant (apache#16040) * [Query Resource Isolation]Interface for Workload Stats Collection (apache#16340) * Interface for Stats Collection * Additional comments * inherit * additional class comments * [Query Resource Isolation] Fix Refresh message (apache#16636) * Fix Refresh message * delete queryworkload message handler * info -> debug logs * reduce logging (apache#16698) * style check * [Query Workload Isolation] Cost-split support (apache#16672) * splits * Cost split * test * propagation entity change & java doc * Propagation scheme review comments * empty commit to trigger build * Reduce log for PerQueryCPUMemResourceUsageAccountant (apache#16642) * [refactor] Switching to RoutingManager for broker request handlers (apache#16442) Co-authored-by: Shaurya Chaturvedi <shauryachats@uber.com> * Fix broker request id generator to avoid generating same id (apache#16661) * Introduce QueryExecutionContext to manage query life cycle (apache#16728) * Introduce QueryExecutionContext to manage query life cycle 2 (apache#16728) --------- Co-authored-by: Rajat Venkatesh <1638298+vrajat@users.noreply.github.com> Co-authored-by: Yash Mayya <yash.mayya@gmail.com> Co-authored-by: Satwik Pachigolla <40644097+satwik-pachigolla@users.noreply.github.com> Co-authored-by: 9aman <35227405+9aman@users.noreply.github.com> Co-authored-by: Gonzalo Ortiz Jaureguizar <gortiz@users.noreply.github.com> Co-authored-by: Vivek Iyer Vaidyanathan <vvivekiyer@gmail.com> Co-authored-by: Xiaotian (Jackie) Jiang <17555551+Jackie-Jiang@users.noreply.github.com> Co-authored-by: Rajat Venkatesh <vrajat@users.noreply.github.com> Co-authored-by: Vivek Iyer Vaidyanathan Iyer <vvaidyanathan@linkedin.com> Co-authored-by: Shaurya Chaturvedi <shauryachats@gmail.com> Co-authored-by: Shaurya Chaturvedi <shauryachats@uber.com>

vrajat mentioned this pull request Jul 18, 2025

Add proactive per-query memory checking to prevent OOM #16378

Open

vrajat force-pushed the rv-tracing-hard-kill-panic branch from b0ba236 to 7aaf0e5 Compare July 22, 2025 06:17

vrajat changed the title ~~Queries now self terminate if in panic mode.~~ Queries now self terminate if in critical or panic mode. Jul 22, 2025

gortiz self-requested a review July 22, 2025 07:07

gortiz reviewed Jul 22, 2025

View reviewed changes

xiangfu0 reviewed Jul 23, 2025

View reviewed changes

pinot-core/src/main/java/org/apache/pinot/core/accounting/PerQueryCPUMemAccountantFactory.java Outdated Show resolved Hide resolved

vrajat force-pushed the rv-tracing-hard-kill-panic branch from 55ef2bd to 2b39345 Compare July 23, 2025 10:17

vrajat marked this pull request as ready for review July 23, 2025 17:32

vrajat changed the title ~~Queries now self terminate if in critical or panic mode.~~ Queries now self terminate if in panic mode. Jul 24, 2025

vrajat and others added 8 commits July 24, 2025 18:58

Queries now self terminate if in panic mode.

74b1f22

Add config test

a57a979

Hard kill on critical level.

ee18d77

Fix configs

6f2b7ea

Separate anchor thread interruption.

c32034b

Checkstyle

a6e8758

Review comments

9fd985a

remove code for critical level

429d0c9

vrajat force-pushed the rv-tracing-hard-kill-panic branch from 6abfc9f to 429d0c9 Compare July 24, 2025 13:28

xiangfu0 added enhancement query performance java Pull requests that update Java code multi-stage Related to the multi-stage query engine labels Jul 24, 2025

xiangfu0 approved these changes Jul 25, 2025

View reviewed changes

xiangfu0 merged commit c1fceb2 into apache:master Jul 25, 2025
35 of 36 checks passed

vrajat deleted the rv-tracing-hard-kill-panic branch July 30, 2025 13:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Queries now self terminate if in panic mode.#16380

Queries now self terminate if in panic mode.#16380
xiangfu0 merged 8 commits intoapache:masterfrom
vrajat:rv-tracing-hard-kill-panic

vrajat commented Jul 18, 2025 •

edited

Loading

Uh oh!

gortiz left a comment

Uh oh!

gortiz Jul 22, 2025

Uh oh!

gortiz Jul 22, 2025

Uh oh!

vrajat Jul 22, 2025

Uh oh!

vrajat Jul 23, 2025

Uh oh!

codecov-commenter commented Jul 22, 2025 •

edited

Loading

Uh oh!

xiangfu0 Jul 23, 2025 •

edited

Loading

Uh oh!

vrajat Jul 23, 2025

Uh oh!

Uh oh!

xiangfu0 commented Jul 24, 2025

Uh oh!

vrajat commented Jul 24, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants


		protected Set<String> _cancelSentQueries;

		protected String _maxMemoryUsageQueryId;

Conversation

vrajat commented Jul 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gortiz left a comment

Choose a reason for hiding this comment

Uh oh!

gortiz Jul 22, 2025

Choose a reason for hiding this comment

Uh oh!

gortiz Jul 22, 2025

Choose a reason for hiding this comment

Uh oh!

vrajat Jul 22, 2025

Choose a reason for hiding this comment

Uh oh!

vrajat Jul 23, 2025

Choose a reason for hiding this comment

Uh oh!

codecov-commenter commented Jul 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

xiangfu0 Jul 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vrajat Jul 23, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

xiangfu0 commented Jul 24, 2025

Uh oh!

vrajat commented Jul 24, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

vrajat commented Jul 18, 2025 •

edited

Loading

codecov-commenter commented Jul 22, 2025 •

edited

Loading

xiangfu0 Jul 23, 2025 •

edited

Loading