-
Notifications
You must be signed in to change notification settings - Fork 1.4k
[federation] Add multi-cluster routing support for SSE queries #17439
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
pinot-core/src/main/java/org/apache/pinot/core/routing/LogicalTableRouteProvider.java
Show resolved
Hide resolved
| : _routingManager; | ||
| // Get the tables hit by the request | ||
| TableRouteInfo routeInfo = routeProvider.getTableRouteInfo(tableName, _tableCache, _routingManager); | ||
| TableRouteInfo routeInfo = routeProvider.getTableRouteInfo(tableName, _tableCache, routingManager); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems it's pretty easy to create a bug in a future code change where instead of passing routingManager someone uses _routingManager.
Maybe rename _routingManager to _defaultRoutingManager and add a comment on the var declaration?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point. The intent was to keep _routingManager semantics unchanged and limit the change to the per-query path. Renaming routingManager to selectedRoutingManager makes the selection explicit and also tells us this path uses a conditional routing manager.
Renaming _routingManager would be much broader given its usage.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #17439 +/- ##
=========================================
Coverage 63.26% 63.26%
Complexity 1474 1474
=========================================
Files 3161 3161
Lines 188390 188427 +37
Branches 28822 28831 +9
=========================================
+ Hits 119185 119216 +31
- Misses 59977 59985 +8
+ Partials 9228 9226 -2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Summary
This PR adds multi-cluster routing support for SSE queries by introducing a new query option
enableMultiClusterRoutingin the SQL query.The PR preserves existing behavior when multi-cluster routing is disabled.
Changes
MultiClusterRoutingContextthrough all broker request handlers to support federated routing.enableMultiClusterRouting, to select the federatedRoutingManagerat query time.PhysicalTableConfig.multiClusterflag.Testing
Added a multi-cluster integration test that validates logical table federation across two clusters and verifies aggregated results with multi-cluster routing enabled.