dkranchii opened a new pull request, #19237:
URL: https://github.com/apache/pinot/pull/19237
## Summary
`QuotaConfig` produces misleading validation errors when a table config has
a bad `storage` or `maxQueriesPerSecond` value. Four defects fixed:
1. **Wrong variable interpolated.** For a bad `maxQueriesPerSecond`, the
code threw `"Invalid 'maxQueriesPerSecond': " + storage`, so the operator saw
the `storage` value (or `null`) instead of the offending QPS input.
2. **Missing exception cause.** Both `IllegalArgumentException` throws
dropped the underlying `NumberFormatException` / parse exception, so stack
traces had no root cause.
3. **Message-less `Preconditions.checkArgument`.**
`Preconditions.checkArgument(_maxQPS > 0)` had no message; a non-positive QPS
produced an `IllegalArgumentException` with a `null` message.
4. **`"Infinity"` bypass.** `Double.parseDouble("Infinity")` returns
`POSITIVE_INFINITY`, which passes `> 0` — an undocumented back-door for
unlimited QPS. `"NaN"` was also silently reaching downstream code.
## Changes
-
`pinot-spi/src/main/java/org/apache/pinot/spi/config/table/QuotaConfig.java`
(+4 / -3):
- Interpolate `maxQueriesPerSecond` (not `storage`) in the QPS error
message.
- Pass the causing exception `e` on both `IllegalArgumentException` throws.
- Give the `Preconditions.checkArgument` a descriptive message that
includes the offending value.
- Tighten the positivity check to `Double.isFinite(_maxQPS) && _maxQPS >
0` so `Infinity`, `-Infinity`, and `NaN` are all rejected.
-
`pinot-spi/src/test/java/org/apache/pinot/spi/config/table/QuotaConfigTest.java`
(+109, tests only) — six new focused tests covering the field name, offending
value, cause chain, zero boundary, non-finite inputs, and the Jackson
deserialization path.
## Backward compatibility
- No public API signature or serialization format is changed.
- Error message tis soft public surface. Callers that string-matched on
`"Invalid 'maxQueriesPerSecond': null"` (the buggy pre-fix message) will need
to update, but the corrected format is what the code clearly intended.
- `"Infinity"` / `"NaN"` inputs now fail validation. If any deployment
relied on `"Infinity"` as a shortcut for "unlimited", they should omit the
field instead (which already means "unlimited" via `INVALID_MAX_QPS = -1.0`).
## Note on overlap with #16016
PR #16016 (a much larger, ~1-year-old feature refactor: "Enable Ratelimiter
Quota with Flexible Configuration") also modifies `QuotaConfig.java` and
`QuotaConfigTest.java`. That PR is a feature refactor with orthogonal concerns;
this PR is a focused bug fix on the existing validation error paths. Happy to
rebase whichever merges second.
## Test plan
- New regression tests in `QuotaConfigTest` covering all four defects, plus
boundary cases (`0`, `Infinity`, `-Infinity`, `NaN`) and the Jackson
deserialization path.
- CI will run `./mvnw -pl pinot-spi -am -Dtest=QuotaConfigTest test` plus
spotless / checkstyle / license.
--
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]
