You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "jineshparakh (via GitHub)" <gi...@apache.org> on 2026/09/15 10:52:39 UTC
[PR] Report BrokerResourceMissing instead of a generic internal error when MSE routing is absent [pinot]
jineshparakh opened a new pull request, #19558:
URL: https://github.com/apache/pinot/pull/19558
## Intent
When a broker holds no routing entry for a table, the two query engines report the same condition
with different error codes.
The single-stage engine returns `BROKER_RESOURCE_MISSING` (410). The multi-stage engine signals it
with a plain precondition failure, which the request handler rewraps as `INTERNAL` (450) because it
only propagates causes that are already a `QueryException`.
So a client sees 450 from one engine and 410 from the other for the identical situation. That
matters because `INTERNAL` is the catch-all for any unexpected planning fault — it tells the caller
nothing actionable and is indistinguishable from a genuine planner bug. `BROKER_RESOURCE_MISSING` is
the specific signal for this case; `QueryErrorCode#isClientError` already documents it as something
that "can be triggered due to issues with EV updates", which is exactly what this is.
In practice the window shows up when a table moves between broker tenants, or a broker is scaled
down: the broker keeps receiving queries for a table it no longer routes. Single-stage clients get a
clear 410 they can react to; multi-stage clients get an opaque 450.
## What this changes
`WorkerManager` reaches the "no routing for this table" conclusion on two planning paths, and both
now throw the typed `BROKER_RESOURCE_MISSING` exception instead of a bare `IllegalStateException`:
| Method | Reached by |
|---|---|
| `assignWorkersToNonPartitionedLeafFragment` | ordinary leaf fragments |
| `calculatePartitionTableInfo` | partitioned / colocated-join plans |
Both are changed because a caller cannot be expected to know which planning path its query took.
Error messages are unchanged, and `MultiStageBrokerRequestHandler` needs no edit — it already passes
a `QueryException` through with its code intact.
### This is precisely the single-stage condition, not an approximation
Both engines end up asking the same question of the same object:
- Single-stage populates its offline/realtime route flags directly from `RoutingManager.routingExists`
and ORs them for a hybrid table.
- Multi-stage's partitioned path makes the identical `routingExists` calls and ORs them.
- Multi-stage's leaf path sees an empty routing map only when the routing table came back null for
every table type — and the broker routing manager returns null **only** when the routing entry is
absent.
That last point also rules out a false positive: a table whose segments are all pruned away still
produces a non-null routing table with an empty server map, so neither site can fire for "no matching
data". Those queries keep returning an empty result rather than an error.
## Scope
`WorkerManager` has 33 precondition checks. Only the two that mean "this table has no routing" are
changed. The others — missing partition info, segments with an invalid partition, no fully replicated
server — describe different conditions and would each need their own mapping decision, so they are
deliberately left alone.
## Compatibility
A routeless table in the multi-stage engine now reports 410 where it previously reported 450. A
client keying on 450 for that case would need to key on 410 — which is already what it does for the
same case on the single-stage engine.
HTTP behaviour is unchanged by default: query errors are returned as HTTP 200 with the code in the
`X-Pinot-Error-Code` header, and only map to a real status when the caller opts in via
`USE_HTTP_STATUS_FOR_ERRORS_HEADER` (where 410 maps to 503 rather than 500).
## Test plan
Two tests added to `WorkerManagerTest`, one per planning path, each asserting the thrown exception
carries `BROKER_RESOURCE_MISSING`:
- `testLeafAssignmentReportsBrokerResourceMissingWhenRoutingIsAbsent`
- `testPartitionedAssignmentReportsBrokerResourceMissingWhenRoutingIsAbsent`
Results:
- `WorkerManagerTest`: **72 tests, 0 failures.**
- Both new tests were verified to be load-bearing: reverting either site to the original precondition
makes the corresponding test fail.
- The four existing tests asserting `IllegalStateException` cover different preconditions (invalid
segment partitions, no fully replicated server, no partition for the table) and are unaffected.
- `checkstyle` and `license` pass on `pinot-query-planner`.
--
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] Report BrokerResourceMissing instead of a generic internal error when MSE routing is absent [pinot]
Posted by "gortiz (via GitHub)" <gi...@apache.org>.
gortiz commented on code in PR #19558:
URL: https://github.com/apache/pinot/pull/19558#discussion_r4015644797
##########
pinot-query-planner/src/main/java/org/apache/pinot/query/routing/WorkerManager.java:
##########
@@ -1785,8 +1789,10 @@ private PartitionTableInfo calculatePartitionTableInfo(String tableName) {
String realtimeTableName = TableNameBuilder.REALTIME.tableNameWithType(tableName);
boolean offlineRoutingExists = _routingManager.routingExists(offlineTableName);
boolean realtimeRoutingExists = _routingManager.routingExists(realtimeTableName);
- Preconditions.checkState(offlineRoutingExists || realtimeRoutingExists, "Routing doesn't exist for table: %s",
- tableName);
+ if (!offlineRoutingExists && !realtimeRoutingExists) {
Review Comment:
`calculatePartitionTableInfo` has a second caller that this change breaks: `inferTableOptions` (line ~1768) wraps it in `catch (IllegalStateException e) { return null; }` so the implicit table-hint path degrades quietly to a shuffled plan. That contract is documented a few lines below this hunk, in the javadoc of `checkNoSegmentsWithInvalidPartition`:
> Throws `IllegalStateException` rather than using `Preconditions` so that the implicit table hint path (`#inferTableOptions`) keeps degrading quietly to a non-partitioned (shuffled) plan.
`QueryException extends PinotRuntimeException extends RuntimeException`, not `IllegalStateException`, so the new throw walks straight out of `inferTableOptions`, out of `PinotImplicitTableHintRule#onMatch`, out of the trait `HepProgram`, and lands in `QueryEnvironment#optimize()` — whose only handler is `catch (Throwable e) { throw QueryErrorCode.QUERY_PLANNING.asException(...) }`, with no `QueryException` passthrough (unlike `validate()`, `toRelation()` and `planQuery()`, which all have one).
Repro on this branch, same environment as the new partitioned test but with the `tableOptions` hint dropped and `inferPartitionHint` enabled:
```
SET inferPartitionHint=true; SELECT col2 FROM testTable WHERE col1 = 'foo'
compile() threw QueryException code=QUERY_PLANNING
msg=Error optimizing query: Routing doesn't exist for table: testTable
```
On master the same query gets past compilation and fails in `planQuery` instead. So for brokers with `pinot.broker.multistage.infer.partition.hint` enabled (or the per-query option), this replaces 450 with **720**, not 410 — and 720 maps to `BAD_REQUEST` and is in `CRITICAL_ERROR_CODES`, so the caller is blamed for it and it stays SLA-critical.
Neither new test can catch this. The partitioned one passes `partition_key`, `partition_function` and `partition_size` together, so `PinotImplicitTableHintRule#getHintOptionsToRewrite` returns null, `matches()` is false and `inferTableOptions` is never called. The leaf one relies on `EmptyTableRoutingManager#routingExists` returning `true` unconditionally, which no real routing manager does when the routing entry is absent — with a faithful stub it would hit this site first.
Suggested fix, ideally both halves:
1. `inferTableOptions`: `catch (IllegalStateException | QueryException e) { return null; }`, so the degrade-quietly contract stays explicit for the new exception type.
2. `optimize()`: add `catch (QueryException e) { throw e; }` so a typed error raised inside a planner rule is not relabelled `QUERY_PLANNING`. That one is a latent bug beyond this PR, but this PR is what makes it reachable.
And a third test — `inferPartitionHint=true`, no hint — asserting `BROKER_RESOURCE_MISSING` comes out of `planQuery()`.
--
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] Report BrokerResourceMissing instead of a generic internal error when MSE routing is absent [pinot]
Posted by "xiangfu0 (via GitHub)" <gi...@apache.org>.
xiangfu0 commented on PR #19558:
URL: https://github.com/apache/pinot/pull/19558#issuecomment-5700463804
The documentation follow-up is merged: https://github.com/pinot-contrib/pinot-docs/pull/1047. The query FAQ now explains the `BROKER_RESOURCE_MISSING` (410) result across SSE and MSE, the previous MSE 450 behavior, and how clients should interpret the error.
--
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] Report BrokerResourceMissing instead of a generic internal error when MSE routing is absent [pinot]
Posted by "gortiz (via GitHub)" <gi...@apache.org>.
gortiz commented on code in PR #19558:
URL: https://github.com/apache/pinot/pull/19558#discussion_r4015819366
##########
pinot-query-planner/src/main/java/org/apache/pinot/query/routing/WorkerManager.java:
##########
@@ -1785,8 +1789,10 @@ private PartitionTableInfo calculatePartitionTableInfo(String tableName) {
String realtimeTableName = TableNameBuilder.REALTIME.tableNameWithType(tableName);
boolean offlineRoutingExists = _routingManager.routingExists(offlineTableName);
boolean realtimeRoutingExists = _routingManager.routingExists(realtimeTableName);
- Preconditions.checkState(offlineRoutingExists || realtimeRoutingExists, "Routing doesn't exist for table: %s",
- tableName);
+ if (!offlineRoutingExists && !realtimeRoutingExists) {
Review Comment:
Following up on my comment above — I built the case and it's worse than I described. It isn't only a worse error code on a query that was failing anyway: with `inferPartitionHint` enabled, **a healthy logical table stops compiling**.
A logical table never has a routing entry — `_routingEntryMap` is populated only by `buildRouting(tableNameWithType)` for physical tables (`BaseBrokerRoutingManager:957`), while `buildRoutingForLogicalTable` installs only time-boundary managers (`:628-650`). It is routed at plan time by `LogicalTableRouteProvider` from `DispatchablePlanVisitor#visitTableScan`, which never consults `routingExists`. So `routingExists` is false for both typed names of a table that is entirely fine, and the implicit-hint rule reaches this check during `compile()`.
A/B on the PR parent vs. this branch, same test, only the commit differs:
```
8228534299 (parent) ac8f9a88 (PR)
SET inferPartitionHint=true; compile OK compile -> QUERY_PLANNING
SELECT col1 FROM logicalTable planQuery OK "Routing doesn't exist for
table: logicalTable"
same query, no hint option compile OK compile OK
planQuery OK planQuery OK
```
The second row is the control. This makes fix (1) — catching `QueryException` in `inferTableOptions` — mandatory rather than defensive, and the extra test is best written on the logical-table case, since that is the one where a query that works today starts failing.
--
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] Report BrokerResourceMissing instead of a generic internal error when MSE routing is absent [pinot]
Posted by "jineshparakh (via GitHub)" <gi...@apache.org>.
jineshparakh commented on code in PR #19558:
URL: https://github.com/apache/pinot/pull/19558#discussion_r4018476062
##########
pinot-query-planner/src/main/java/org/apache/pinot/query/routing/WorkerManager.java:
##########
@@ -1785,8 +1789,10 @@ private PartitionTableInfo calculatePartitionTableInfo(String tableName) {
String realtimeTableName = TableNameBuilder.REALTIME.tableNameWithType(tableName);
boolean offlineRoutingExists = _routingManager.routingExists(offlineTableName);
boolean realtimeRoutingExists = _routingManager.routingExists(realtimeTableName);
- Preconditions.checkState(offlineRoutingExists || realtimeRoutingExists, "Routing doesn't exist for table: %s",
- tableName);
+ if (!offlineRoutingExists && !realtimeRoutingExists) {
Review Comment:
Crazy find @gortiz 🚀
Fixed it.
--
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] Report BrokerResourceMissing instead of a generic internal error when MSE routing is absent [pinot]
Posted by "gortiz (via GitHub)" <gi...@apache.org>.
gortiz merged PR #19558:
URL: https://github.com/apache/pinot/pull/19558
--
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] Report BrokerResourceMissing instead of a generic internal error when MSE routing is absent [pinot]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #19558:
URL: https://github.com/apache/pinot/pull/19558#issuecomment-5679937911
## [Codecov](https://app.codecov.io/gh/apache/pinot/pull/19558?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 `0%` with `4 lines` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 39.43%. Comparing base ([`8228534`](https://app.codecov.io/gh/apache/pinot/commit/8228534299a796107b6f9f4e1e39ebc94f4f866e?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`a078a22`](https://app.codecov.io/gh/apache/pinot/commit/a078a2256b9d1020ac2e923907e3ec5878754806?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
| [Files with missing lines](https://app.codecov.io/gh/apache/pinot/pull/19558?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [.../org/apache/pinot/query/routing/WorkerManager.java](https://app.codecov.io/gh/apache/pinot/pull/19558?src=pr&el=tree&filepath=pinot-query-planner%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fquery%2Frouting%2FWorkerManager.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtcXVlcnktcGxhbm5lci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvcXVlcnkvcm91dGluZy9Xb3JrZXJNYW5hZ2VyLmphdmE=) | 0.00% | [3 Missing and 1 partial :warning: ](https://app.codecov.io/gh/apache/pinot/pull/19558?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 (8228534) and HEAD (a078a22). Click for more details.
>
> <details><summary>HEAD has 11 uploads less than BASE</summary>
>
>| Flag | BASE (8228534) | HEAD (a078a22) |
>|------|------|------|
>|unittests1|1|0|
>|java-25|5|3|
>|temurin|5|3|
>|lane-a|2|1|
>|integration|4|2|
>|integration1|2|0|
>|lane-b|2|1|
></details>
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #19558 +/- ##
=============================================
- Coverage 57.91% 39.43% -18.48%
- Complexity 7 1443 +1436
=============================================
Files 2692 3499 +807
Lines 164795 225976 +61181
Branches 26764 35678 +8914
=============================================
- Hits 95444 89121 -6323
- Misses 61295 128788 +67493
- Partials 8056 8067 +11
```
| [Flag](https://app.codecov.io/gh/apache/pinot/pull/19558/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [integration](https://app.codecov.io/gh/apache/pinot/pull/19558/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `0.00% <ø> (-100.00%)` | :arrow_down: |
| [integration1](https://app.codecov.io/gh/apache/pinot/pull/19558/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/19558/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `0.00% <ø> (ø)` | |
| [java-25](https://app.codecov.io/gh/apache/pinot/pull/19558/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `39.43% <0.00%> (-18.48%)` | :arrow_down: |
| [lane-a](https://app.codecov.io/gh/apache/pinot/pull/19558/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `0.00% <ø> (-100.00%)` | :arrow_down: |
| [lane-b](https://app.codecov.io/gh/apache/pinot/pull/19558/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `0.00% <ø> (ø)` | |
| [temurin](https://app.codecov.io/gh/apache/pinot/pull/19558/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `39.43% <0.00%> (-18.48%)` | :arrow_down: |
| [unittests](https://app.codecov.io/gh/apache/pinot/pull/19558/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `39.43% <0.00%> (-18.48%)` | :arrow_down: |
| [unittests1](https://app.codecov.io/gh/apache/pinot/pull/19558/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [unittests2](https://app.codecov.io/gh/apache/pinot/pull/19558/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `39.43% <0.00%> (?)` | |
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 Harness](https://app.codecov.io/gh/apache/pinot/pull/19558?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] Report BrokerResourceMissing instead of a generic internal error when MSE routing is absent [pinot]
Posted by "jineshparakh (via GitHub)" <gi...@apache.org>.
jineshparakh commented on PR #19558:
URL: https://github.com/apache/pinot/pull/19558#issuecomment-5685041224
@gortiz can you please re-review?
Thanks!
--
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