You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "spapin (via GitHub)" <gi...@apache.org> on 2026/09/25 23:06:23 UTC
[PR] Fix duplicate QueryErrorCode id 429 so quota rejections decode as TOO_MANY_REQUESTS [pinot]
spapin opened a new pull request, #19676:
URL: https://github.com/apache/pinot/pull/19676
## Problem
`QueryErrorCode.TOO_MANY_REQUESTS` and `QueryErrorCode.WORKLOAD_BUDGET_EXCEEDED` both use id 429. The static initializer fills `BY_ID` in declaration order, so the later constant wins and `fromErrorCode(429)` returns `WORKLOAD_BUDGET_EXCEEDED`.
The broker rejects application, database and table query quota with `TOO_MANY_REQUESTS` in `BaseBrokerRequestHandler`, `BaseSingleStageBrokerRequestHandler` and `MultiStageBrokerRequestHandler`. Anything that decodes that response's error code gets the wrong constant back.
## Impact
- **Broker metrics.** `PinotClientRequest` passes every response to `BrokerResponse.emitBrokerResponseMetrics`, which decodes each exception with `fromErrorCode` and increments the matching `QUERY_ERROR_<enum name>` meter. Quota rejections are therefore counted under `QUERY_ERROR_WORKLOAD_BUDGET_EXCEEDED`, and `QUERY_ERROR_TOO_MANY_REQUESTS` never increments.
- **Client/server classification.** `isClientError()` is true for `TOO_MANY_REQUESTS` and false for `WORKLOAD_BUDGET_EXCEEDED`, so a decoded 429 looks like a server error. Inside Pinot the only caller, `AsyncQueryResponse`, sees only server-returned codes, which never include 429, so this affects external callers of `fromErrorCode(code).isClientError()`.
The response JSON (`errorCode: 429`) and the HTTP status are the same either way.
## Fix
Move `WORKLOAD_BUDGET_EXCEEDED` to the unused id 430 and add it to `queryErrorCodeMap` in `Query.tsx`, as the enum header asks.
Nothing sends `QueryErrorCode.WORKLOAD_BUDGET_EXCEEDED` today. Workload admission rejections (`WorkloadScheduler`) and workload kills (`WorkloadResourceAggregator`) both send `SERVER_RESOURCE_LIMIT_EXCEEDED`. The `WORKLOAD_BUDGET_EXCEEDED` names in those classes are `ServerMeter`/`BrokerMeter` metrics, not this error code. Error codes cross the wire as integer ids, so renumbering an unsent constant does not affect mixed-version clusters.
The constant is renumbered rather than removed because `pinot-spi` is a public SPI and the workload feature intended to use it. #16018 added it, and the review there asked for it to be propagated when workloads terminate queries. #16876 would have done that but was closed in favour of #16728, and #17124 later moved admission rejections to `SERVER_RESOURCE_LIMIT_EXCEEDED`. If the workload owners (@praveenc7, @vvivekiyer, @Jackie-Jiang) would rather drop it, that is an easy change to this PR.
## Verification
New `QueryErrorCodeTest` checks that every id is unique, that `fromErrorCode(code.getId()) == code` for every constant, and that 429 decodes to `TOO_MANY_REQUESTS` as a client error. All three failed before the fix:
```
testIdsAreUnique: Error code id 429 is shared by TOO_MANY_REQUESTS and WORKLOAD_BUDGET_EXCEEDED
testFromErrorCodeReturnsConstantWithThatId: expected [TOO_MANY_REQUESTS] but found [WORKLOAD_BUDGET_EXCEEDED]
testTooManyRequestsDecodesAsClientError: expected [TOO_MANY_REQUESTS] but found [WORKLOAD_BUDGET_EXCEEDED]
Tests run: 3, Failures: 3, Errors: 0, Skipped: 0
```
- JDK 25: the full `pinot-spi` suite passes, **843 tests**, no failures or skips.
- The full reactor compiles, main and test sources.
- `spotless:apply`, `license:format`, `checkstyle:check` and `license:check` pass on `pinot-spi` and `pinot-controller` without changes. A deprecation-enabled `test-compile` of `pinot-spi` shows no warnings on added lines.
## Scope
User-visible: broker query-quota rejections now count under `QUERY_ERROR_TOO_MANY_REQUESTS` (`queryErrorTooManyRequests`) instead of `QUERY_ERROR_WORKLOAD_BUDGET_EXCEEDED` (`queryErrorWorkloadBudgetExceeded`). Dashboards or alerts on the old meter should move to the new one.
Open questions, not changed here:
- Should `WORKLOAD_BUDGET_EXCEEDED` be in `isClientError()`? `SERVER_RESOURCE_LIMIT_EXCEEDED`, which workloads send today, is.
- Should workload admission rejections and kills send `WORKLOAD_BUDGET_EXCEEDED`?
Generated-by: Claude Code (Claude Opus 5.5)
--
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] Fix duplicate QueryErrorCode id 429 so quota rejections decode as TOO_MANY_REQUESTS [pinot]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #19676:
URL: https://github.com/apache/pinot/pull/19676#issuecomment-5841306829
## [Codecov](https://app.codecov.io/gh/apache/pinot/pull/19676?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:white_check_mark: All modified and coverable lines are covered by tests.
:white_check_mark: Project coverage is 100.00%. Comparing base ([`f626b0c`](https://app.codecov.io/gh/apache/pinot/commit/f626b0c320a5f194640f442ac227e8ac7f79a64e?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`f57620e`](https://app.codecov.io/gh/apache/pinot/commit/f57620ea6f4916beb9cec3b24126b6490cb677b6?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #19676 +/- ##
==============================================
+ Coverage 68.06% 100.00% +31.93%
+ Complexity 1450 6 -1444
==============================================
Files 3511 3 -3508
Lines 227548 6 -227542
Branches 35998 0 -35998
==============================================
- Hits 154883 6 -154877
+ Misses 60501 0 -60501
+ Partials 12164 0 -12164
```
| [Flag](https://app.codecov.io/gh/apache/pinot/pull/19676/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/19676/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `100.00% <ø> (ø)` | |
| [integration1](https://app.codecov.io/gh/apache/pinot/pull/19676/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `100.00% <ø> (ø)` | |
| [integration2](https://app.codecov.io/gh/apache/pinot/pull/19676/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [java-25](https://app.codecov.io/gh/apache/pinot/pull/19676/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `100.00% <ø> (+31.93%)` | :arrow_up: |
| [lane-a](https://app.codecov.io/gh/apache/pinot/pull/19676/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `100.00% <ø> (ø)` | |
| [lane-b](https://app.codecov.io/gh/apache/pinot/pull/19676/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/19676/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `100.00% <ø> (+31.93%)` | :arrow_up: |
| [unittests](https://app.codecov.io/gh/apache/pinot/pull/19676/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [unittests1](https://app.codecov.io/gh/apache/pinot/pull/19676/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/19676/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
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/19676?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] Fix duplicate QueryErrorCode id 429 so quota rejections decode as TOO_MANY_REQUESTS [pinot]
Posted by "xiangfu0 (via GitHub)" <gi...@apache.org>.
xiangfu0 commented on PR #19676:
URL: https://github.com/apache/pinot/pull/19676#issuecomment-5876713352
Documented the corrected query-quota error code and metric migration in merged pinot-docs PR: https://github.com/pinot-contrib/pinot-docs/pull/1074
--
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] Fix duplicate QueryErrorCode id 429 so quota rejections decode as TOO_MANY_REQUESTS [pinot]
Posted by "xiangfu0 (via GitHub)" <gi...@apache.org>.
xiangfu0 merged PR #19676:
URL: https://github.com/apache/pinot/pull/19676
--
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