[refactor] Switching to RoutingManager for broker request handlers by shauryachats · Pull Request #16442 · apache/pinot

shauryachats · 2025-07-28T23:40:02Z

This PR updates broker request handlers to use the RoutingManager interface instead of BrokerRoutingManager to accommodate other RoutingManagers in the future and make the codebase more modular.

It also removes the dependency of routingManager for retrieving the query timeout for a table through the TableConfig by retrieving the table config directly from _tableCache in BaseSingleStageBrokerRequestHandler.

codecov-commenter · 2025-07-29T00:29:34Z

Codecov Report

❌ Patch coverage is 20.00000% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 63.27%. Comparing base (4f46771) to head (c45f596).

Files with missing lines	Patch %	Lines
...sthandler/BaseSingleStageBrokerRequestHandler.java	20.00%	2 Missing and 2 partials ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##             master   #16442      +/-   ##
============================================
+ Coverage     63.24%   63.27%   +0.03%     
+ Complexity     1363     1362       -1     
============================================
  Files          3010     3010              
  Lines        174247   174249       +2     
  Branches      26680    26682       +2     
============================================
+ Hits         110196   110253      +57     
+ Misses        55644    55584      -60     
- Partials       8407     8412       +5

Flag	Coverage Δ
custom-integration1	`100.00% <ø> (?)`
integration	`100.00% <ø> (ø)`
integration1	`100.00% <ø> (ø)`
integration2	`0.00% <ø> (ø)`
java-11	`63.24% <20.00%> (+0.02%)`	⬆️
java-21	`63.20% <20.00%> (-0.01%)`	⬇️
temurin	`63.27% <20.00%> (+0.03%)`	⬆️
unittests	`63.26% <20.00%> (+0.03%)`	⬆️
unittests1	`56.39% <ø> (-0.01%)`	⬇️
unittests2	`33.32% <20.00%> (+0.03%)`	⬆️

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.

ankitsultana · 2025-08-02T16:45:56Z

pinot-core/src/main/java/org/apache/pinot/core/routing/RoutingManager.java

+   * Returns {@code null} if the table does not have a query timeout set.
+   */
+  @Nullable
+  Long getQueryTimeoutMs(String tableNameWithType);


query timeout should be associated with something like a RequestContext. Can we avoid this method?

Makes sense. Removed the dependency of routingManager for retrieving query timeout for a table through the TableConfig by calling it directly in BaseSingleStageBrokerRequestHandler.

ankitsultana · 2025-08-05T16:47:34Z

...rc/main/java/org/apache/pinot/broker/requesthandler/BaseSingleStageBrokerRequestHandler.java

    } else {
-      Long tableLevelTimeoutMs = _routingManager.getQueryTimeoutMs(tableNameWithType);
-      if (tableLevelTimeoutMs != null) {
+      TableConfig tableConfig = _tableCache.getTableConfig(tableNameWithType);


before we were getting timeout from RoutingEntry, which was getting created async and was getting the timeout from Table Config retrieved from ZKMetadataProvider.

Now we are getting it from tableCache synchronously in the query path. I suppose this should be fine, but just leaving a note here to call it out.

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

shauryachats force-pushed the ref_brokerrouting branch 2 times, most recently from f9cc0cb to 636a16c Compare July 29, 2025 17:55

Jackie-Jiang added the refactor label Jul 31, 2025

shauryachats force-pushed the ref_brokerrouting branch from 636a16c to 161784b Compare August 1, 2025 22:33

ankitsultana reviewed Aug 2, 2025

View reviewed changes

shauryachats added 2 commits August 4, 2025 23:48

[refactor] Switching to RoutingManager for broker request handlers

5cccb56

Removed getQueryTimeout from RoutingManager

c45f596

shauryachats force-pushed the ref_brokerrouting branch from 161784b to c45f596 Compare August 4, 2025 23:51

ankitsultana approved these changes Aug 5, 2025

View reviewed changes

ankitsultana reviewed Aug 5, 2025

View reviewed changes

ankitsultana merged commit 123dbfe into apache:master Aug 5, 2025
18 checks passed

shauryachats deleted the ref_brokerrouting branch August 5, 2025 18:50

dang-stripe mentioned this pull request Jan 8, 2026

[multistage] Reset GRPC connection backoff when server is re-enabled #17466

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[refactor] Switching to RoutingManager for broker request handlers#16442

[refactor] Switching to RoutingManager for broker request handlers#16442
ankitsultana merged 2 commits intoapache:masterfrom
shauryachats:ref_brokerrouting

shauryachats commented Jul 28, 2025 •

edited

Loading

Uh oh!

codecov-commenter commented Jul 29, 2025 •

edited

Loading

Uh oh!

ankitsultana Aug 2, 2025

Uh oh!

shauryachats Aug 4, 2025

Uh oh!

ankitsultana Aug 5, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

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

Uh oh!

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

ankitsultana Aug 2, 2025

Choose a reason for hiding this comment

Uh oh!

shauryachats Aug 4, 2025

Choose a reason for hiding this comment

Uh oh!

ankitsultana Aug 5, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

shauryachats commented Jul 28, 2025 •

edited

Loading

codecov-commenter commented Jul 29, 2025 •

edited

Loading