Use stats cache on error instead of the chained mechanism by gortiz · Pull Request #15992 · apache/pinot

gortiz · 2025-06-04T11:02:41Z

This PR changes the mechanism used to obtain stats on error. Instead of using the normal mailbox mechanism, it uses the alternative mode used on timeout. This increases the precision in cases where more than one worker fails (ie an invalid cast) or in queries with more than a few stages.

codecov-commenter · 2025-06-04T11:48:54Z

Codecov Report

Attention: Patch coverage is 13.33333% with 13 lines in your changes missing coverage. Please review.

Project coverage is 63.43%. Comparing base (1a476de) to head (e8f9360).
Report is 206 commits behind head on master.

Files with missing lines	Patch %	Lines
.../pinot/query/service/dispatch/QueryDispatcher.java	13.33%	12 Missing and 1 partial ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##             master   #15992      +/-   ##
============================================
+ Coverage     62.90%   63.43%   +0.53%     
+ Complexity     1386     1357      -29     
============================================
  Files          2867     2912      +45     
  Lines        163354   166955    +3601     
  Branches      24952    25537     +585     
============================================
+ Hits         102755   105916    +3161     
- Misses        52847    53028     +181     
- Partials       7752     8011     +259

Flag	Coverage Δ
custom-integration1	`100.00% <ø> (ø)`
integration	`100.00% <ø> (ø)`
integration1	`100.00% <ø> (ø)`
integration2	`0.00% <ø> (ø)`
java-11	`63.41% <13.33%> (+0.54%)`	⬆️
java-21	`63.38% <13.33%> (+0.55%)`	⬆️
skip-bytebuffers-false	`?`
skip-bytebuffers-true	`?`
temurin	`63.43% <13.33%> (+0.53%)`	⬆️
unittests	`63.43% <13.33%> (+0.53%)`	⬆️
unittests1	`56.51% <13.33%> (+0.68%)`	⬆️
unittests2	`33.45% <0.00%> (-0.12%)`	⬇️

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.

Jackie-Jiang · 2025-06-04T19:21:54Z

pinot-query-runtime/src/main/java/org/apache/pinot/query/service/dispatch/QueryDispatcher.java

+        MultiStageQueryStats statsFromCancel = cancelWithStats(requestId, servers);
+        return result.withStats(statsFromCancel);
+      }
+      cancel(requestId, servers);


Why do we cancel here?

This is useful to clean up the stats cache and solves the TODO below. I've just pushed a new version that is simpler to read (and only tries to cancel once).

This cancel method sends an asynchronous cancel request without waiting for a response, so it should be very inexpensive compared to all other GRPC messages we send (ie, rows sent using mailboxes or the plan itself).

gortiz · 2025-06-05T13:46:37Z

Probably we should merge #16010 before this one. Otherwise we are going to receive too many useless log errors

Copilot

Pull Request Overview

This PR updates the error-handling path to use the stats cache directly on failure rather than relying on the chained mailbox mechanism.

Directly cancel and fetch stats via cancelWithStats when a processing exception occurs.
Introduce QueryResult.withStats to merge new stats into an existing result.
Simplify cancellation logic in submitAndReduce, removing the old Throwable catch and redundant cleanup.

Comments suppressed due to low confidence (3)

pinot-query-runtime/src/main/java/org/apache/pinot/query/service/dispatch/QueryDispatcher.java:171

No existing tests cover the new error stats path when result.getProcessingException() is non-null. Add a unit test to verify that withStats correctly incorporates stats on failure.

if (result.getProcessingException() != null) {

pinot-query-runtime/src/main/java/org/apache/pinot/query/service/dispatch/QueryDispatcher.java:791

[nitpick] The new withStats method is public but lacks a Javadoc comment explaining its behavior and intended usage. Adding documentation will improve maintainability.

public QueryResult withStats(MultiStageQueryStats newQueryStats) {

pinot-query-runtime/src/main/java/org/apache/pinot/query/service/dispatch/QueryDispatcher.java:182

The cleanup of _serversByQuery (previously removed) is missing; without it, entries may accumulate when query cancellation is disabled. Consider restoring or centralizing the removal to avoid memory leaks.

if (!cancelled) {

Jackie-Jiang · 2025-06-13T22:16:36Z

pinot-query-runtime/src/main/java/org/apache/pinot/query/service/dispatch/QueryDispatcher.java

-      if (isQueryCancellationEnabled()) {
-        _serversByQuery.remove(requestId);
+      if (!cancelled) {
+        cancel(requestId, servers);


Sending one more request to each server on happy path does add overhead. Ideally we want to avoid this overhead. This is not a blocker, but we should try to figure out a way to minimize the overhead for happy path

…of the chained mechanism (apache#15992)" This reverts commit 3158efa

…ache#15992)" (apache#370) This reverts commit 3158efa.

…of the chained mechanism (apache#15992)" This reverts commit 3158efa

…5992)" (#17305) * Revert QueryDispatcher changes for "Use stats cache on error instead of the chained mechanism (#15992)" This reverts commit 3158efa * Fix test

* [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>

gortiz added observability multi-stage Related to the multi-stage query engine labels Jun 4, 2025

gortiz requested review from Jackie-Jiang and yashmayya June 4, 2025 11:02

Use stats cache on error instead of the chained mechanism

16851af

gortiz force-pushed the use-stats-cache-on-error branch from 1e29882 to 16851af Compare June 4, 2025 11:06

Jackie-Jiang reviewed Jun 4, 2025

View reviewed changes

Simplify the cancellation of queries on finish

a3e8d5a

mark query as cancelled correctly

e8f9360

gortiz requested a review from Copilot June 5, 2025 14:00

Copilot AI reviewed Jun 5, 2025

View reviewed changes

Jackie-Jiang approved these changes Jun 13, 2025

View reviewed changes

gortiz merged commit 3158efa into apache:master Jun 16, 2025
18 checks passed

gortiz deleted the use-stats-cache-on-error branch June 16, 2025 13:15

songwdfu pushed a commit to songwdfu/pinot that referenced this pull request Jun 18, 2025

Use stats cache on error instead of the chained mechanism (apache#15992)

04be7da

gortiz added a commit to gortiz/pinot that referenced this pull request Dec 2, 2025

Revert QueryDispatcher changes for "Use stats cache on error instead …

78052f1

…of the chained mechanism (apache#15992)" This reverts commit 3158efa

gortiz mentioned this pull request Dec 2, 2025

Revert "Use stats cache on error instead of the chained mechanism (#15992)" #17305

Merged

xiangfu0 pushed a commit to xiangfu0/pinot that referenced this pull request Dec 6, 2025

Revert "Use stats cache on error instead of the chained mechanism (ap…

3834b82

…ache#15992)" (apache#370) This reverts commit 3158efa.

gortiz added a commit to gortiz/pinot that referenced this pull request Dec 22, 2025

Revert QueryDispatcher changes for "Use stats cache on error instead …

7913d49

…of the chained mechanism (apache#15992)" This reverts commit 3158efa

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use stats cache on error instead of the chained mechanism#15992

Use stats cache on error instead of the chained mechanism#15992
gortiz merged 3 commits intoapache:masterfrom
gortiz:use-stats-cache-on-error

gortiz commented Jun 4, 2025

Uh oh!

codecov-commenter commented Jun 4, 2025 •

edited

Loading

Uh oh!

Jackie-Jiang Jun 4, 2025

Uh oh!

gortiz Jun 5, 2025

Uh oh!

gortiz commented Jun 5, 2025

Uh oh!

Copilot AI left a comment

Uh oh!

Jackie-Jiang Jun 13, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

gortiz commented Jun 4, 2025

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

Jackie-Jiang Jun 4, 2025

Choose a reason for hiding this comment

Uh oh!

gortiz Jun 5, 2025

Choose a reason for hiding this comment

Uh oh!

gortiz commented Jun 5, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Uh oh!

Jackie-Jiang Jun 13, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

codecov-commenter commented Jun 4, 2025 •

edited

Loading