This is an automated email from the ASF dual-hosted git repository.
Jackie-Jiang pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/pinot.git
The following commit(s) were added to refs/heads/master by this push:
new 388bc64d5c8 Fix misleading QuotaConfig error messages and reject
non-finite maxQueriesPerSecond (#19237)
388bc64d5c8 is described below
commit 388bc64d5c89265ba0b0ab70c2b59af81d3a7862
Author: Deepak kumar <[email protected]>
AuthorDate: Fri Aug 14 15:49:51 2026 -0700
Fix misleading QuotaConfig error messages and reject non-finite
maxQueriesPerSecond (#19237)
---
.../apache/pinot/spi/config/table/QuotaConfig.java | 7 +-
.../pinot/spi/config/table/QuotaConfigTest.java | 109 +++++++++++++++++++++
2 files changed, 113 insertions(+), 3 deletions(-)
diff --git
a/pinot-spi/src/main/java/org/apache/pinot/spi/config/table/QuotaConfig.java
b/pinot-spi/src/main/java/org/apache/pinot/spi/config/table/QuotaConfig.java
index 487cc7dbf38..53ce22a835b 100644
--- a/pinot-spi/src/main/java/org/apache/pinot/spi/config/table/QuotaConfig.java
+++ b/pinot-spi/src/main/java/org/apache/pinot/spi/config/table/QuotaConfig.java
@@ -50,7 +50,7 @@ public class QuotaConfig extends BaseJsonConfig {
try {
_storageInBytes = DataSizeUtils.toBytes(storage);
} catch (Exception e) {
- throw new IllegalArgumentException("Invalid 'storage': " + storage);
+ throw new IllegalArgumentException("Invalid 'storage': " + storage, e);
}
_storage = DataSizeUtils.fromBytes(_storageInBytes);
} else {
@@ -60,9 +60,10 @@ public class QuotaConfig extends BaseJsonConfig {
if (maxQueriesPerSecond != null) {
try {
_maxQPS = Double.parseDouble(maxQueriesPerSecond);
- Preconditions.checkArgument(_maxQPS > 0);
+ Preconditions.checkArgument(Double.isFinite(_maxQPS) && _maxQPS > 0,
+ "'maxQueriesPerSecond' must be a positive finite number, got: %s",
_maxQPS);
} catch (Exception e) {
- throw new IllegalArgumentException("Invalid 'maxQueriesPerSecond': " +
storage);
+ throw new IllegalArgumentException("Invalid 'maxQueriesPerSecond': " +
maxQueriesPerSecond, e);
}
_maxQueriesPerSecond = Double.toString(_maxQPS);
} else {
diff --git
a/pinot-spi/src/test/java/org/apache/pinot/spi/config/table/QuotaConfigTest.java
b/pinot-spi/src/test/java/org/apache/pinot/spi/config/table/QuotaConfigTest.java
index 0de6b0b7dc3..a2aabf79097 100644
---
a/pinot-spi/src/test/java/org/apache/pinot/spi/config/table/QuotaConfigTest.java
+++
b/pinot-spi/src/test/java/org/apache/pinot/spi/config/table/QuotaConfigTest.java
@@ -24,7 +24,11 @@ import org.apache.pinot.spi.utils.JsonUtils;
import org.testng.annotations.Test;
import static org.testng.Assert.assertEquals;
+import static org.testng.Assert.assertFalse;
+import static org.testng.Assert.assertNotNull;
import static org.testng.Assert.assertNull;
+import static org.testng.Assert.assertTrue;
+import static org.testng.Assert.expectThrows;
import static org.testng.Assert.fail;
@@ -113,6 +117,111 @@ public class QuotaConfigTest {
}
}
+ @Test
+ public void testInvalidStorageMessageAndCause() {
+ IllegalArgumentException e =
+ expectThrows(IllegalArgumentException.class, () -> new
QuotaConfig("124GB3GB", null));
+ String msg = e.getMessage();
+ assertTrue(msg.contains("storage"), "Error message should reference the
'storage' field, got: " + msg);
+ assertTrue(msg.contains("124GB3GB"), "Error message should include the
offending value, got: " + msg);
+ assertNotNull(e.getCause(), "Underlying parse exception should be
preserved as the cause");
+ }
+
+ @Test
+ public void testInvalidMaxQPSMessageReferencesCorrectField() {
+ // Verify the field name AND the offending value are both reported
correctly.
+ // Regression: previously the message was "Invalid 'maxQueriesPerSecond':
" + storage, i.e. it
+ // interpolated the wrong variable and reported 'null' (or a valid
'storage' value) instead of
+ // the actual bad maxQueriesPerSecond input.
+ IllegalArgumentException e =
+ expectThrows(IllegalArgumentException.class, () -> new
QuotaConfig(null, "InvalidQpsQuota"));
+ String msg = e.getMessage();
+ assertTrue(msg.contains("maxQueriesPerSecond"),
+ "Error message should reference the 'maxQueriesPerSecond' field, got:
" + msg);
+ assertTrue(msg.contains("InvalidQpsQuota"),
+ "Error message should include the offending value, got: " + msg);
+ assertNotNull(e.getCause(), "Underlying parse exception should be
preserved as the cause");
+
+ // Also verify the case where 'storage' is set alongside a bad
maxQueriesPerSecond. The old code
+ // used to render the storage value here, misleading the operator into
thinking storage was invalid.
+ IllegalArgumentException withStorage =
+ expectThrows(IllegalArgumentException.class, () -> new
QuotaConfig("100G", "InvalidQpsQuota"));
+ String withStorageMsg = withStorage.getMessage();
+ assertTrue(withStorageMsg.contains("InvalidQpsQuota"),
+ "Error message should include the offending maxQueriesPerSecond value,
got: " + withStorageMsg);
+ assertFalse(withStorageMsg.contains("100G"),
+ "Error message must not report the (valid) 'storage' value, got: " +
withStorageMsg);
+ }
+
+ @Test
+ public void testNonPositiveMaxQPSMessageIsInformative() {
+ // Preconditions.checkArgument previously had no message, producing a
null-message
+ // IllegalArgumentException wrapped as "Invalid 'maxQueriesPerSecond':
null".
+ IllegalArgumentException e =
+ expectThrows(IllegalArgumentException.class, () -> new
QuotaConfig(null, "-1.0"));
+ String msg = e.getMessage();
+ assertTrue(msg.contains("maxQueriesPerSecond") && msg.contains("-1.0"),
+ "Outer message should reference the field and offending value, got: "
+ msg);
+ Throwable cause = e.getCause();
+ assertNotNull(cause, "Preconditions.checkArgument failure should be
preserved as the cause");
+ assertTrue(cause instanceof IllegalArgumentException,
+ "Cause should be the IllegalArgumentException thrown by
Preconditions.checkArgument, got: " + cause);
+ String causeMsg = cause.getMessage();
+ assertNotNull(causeMsg, "Preconditions.checkArgument should carry a
non-null message");
+ assertTrue(causeMsg.contains("maxQueriesPerSecond"),
+ "Cause message should reference the field, got: " + causeMsg);
+ assertTrue(causeMsg.contains("-1.0"), "Cause message should include the
offending value, got: " + causeMsg);
+ }
+
+ @Test
+ public void testZeroMaxQPSRejected() {
+ // Zero must be rejected by the '_maxQPS > 0' check.
+ IllegalArgumentException e = expectThrows(IllegalArgumentException.class,
() -> new QuotaConfig(null, "0"));
+ assertTrue(e.getMessage().contains("maxQueriesPerSecond") &&
e.getMessage().contains("0"),
+ "Error message should reference the field and offending value, got: "
+ e.getMessage());
+ }
+
+ @Test
+ public void testNonFiniteMaxQPSRejected() {
+ // 'Infinity' parses to Double.POSITIVE_INFINITY, which passes '> 0'.
Without the isFinite guard
+ // this becomes an undocumented back-door for unlimited QPS. Both Infinity
and NaN must be rejected.
+ for (String bad : new String[] {"Infinity", "-Infinity", "NaN"}) {
+ IllegalArgumentException e =
+ expectThrows(IllegalArgumentException.class, () -> new
QuotaConfig(null, bad));
+ String msg = e.getMessage();
+ assertTrue(msg.contains("maxQueriesPerSecond"),
+ "Error message should reference the field for input '" + bad + "',
got: " + msg);
+ assertTrue(msg.contains(bad),
+ "Error message should include the offending value '" + bad + "',
got: " + msg);
+ }
+ }
+
+ @Test
+ public void testInvalidQuotaThroughJacksonSurfacesFieldAndValue()
+ throws IOException {
+ // Exercise the operator-facing path (JSON -> POJO). The
IllegalArgumentException is wrapped by
+ // Jackson; we walk the cause chain and assert the same invariants: field
name + offending value.
+ String json = "{\"maxQueriesPerSecond\" : \"InvalidQpsQuota\"}";
+ Exception thrown = expectThrows(Exception.class, () ->
JsonUtils.stringToObject(json, QuotaConfig.class));
+
+ boolean foundField = false;
+ boolean foundValue = false;
+ for (Throwable t = thrown; t != null; t = t.getCause()) {
+ String tm = t.getMessage();
+ if (tm == null) {
+ continue;
+ }
+ if (tm.contains("maxQueriesPerSecond")) {
+ foundField = true;
+ }
+ if (tm.contains("InvalidQpsQuota")) {
+ foundValue = true;
+ }
+ }
+ assertTrue(foundField && foundValue,
+ "Deserialization error chain should mention 'maxQueriesPerSecond' and
'InvalidQpsQuota': " + thrown);
+ }
+
@Test
public void testSerDe()
throws IOException {
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
