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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
