xiangfu0 commented on code in PR #19838:
URL: https://github.com/apache/pinot/pull/19838#discussion_r4278299289
##########
pinot-broker/src/main/java/org/apache/pinot/broker/queryquota/HelixExternalViewBasedQueryQuotaManager.java:
##########
@@ -222,6 +225,7 @@ public void dropTableQueryQuota(String
physicalOrLogicalTable) {
/// @param physicalOrLogicalTable physical or logical table name.
private void removeRateLimiter(String physicalOrLogicalTable) {
_rateLimiterMap.remove(physicalOrLogicalTable);
+ _enforcementModeOverrideMap.remove(physicalOrLogicalTable);
Review Comment:
Nice that `removeRateLimiter()` keeps the override map in sync. The three
`it.remove()` sites in `processQueryRateLimitingExternalViewChange` (no broker
resource / deleted config / invalid quota) drop from `_rateLimiterMap` but not
from `_enforcementModeOverrideMap`. It's harmless today since reads are gated
on `_rateLimiterMap.containsKey(...)`, but it leaves stale override entries
that grow with table churn — worth removing from both maps there (and in the
test-only `cleanUpRateLimiterMap()`) for consistency.
##########
pinot-broker/src/main/java/org/apache/pinot/broker/queryquota/HelixExternalViewBasedQueryQuotaManager.java:
##########
@@ -235,13 +239,41 @@ private QuotaConfig
getQuotaConfigFromPropertyStore(String tableNameWithType) {
return tableConfig.getQuotaConfig();
}
+ @Override
+ public QuotaEnforcementMode getQueryQuotaEnforcementMode(String tableName) {
+ List<String> entityNames =
TableNameBuilder.getTableTypeFromTableName(tableName) != null ?
List.of(tableName)
+ : List.of(tableName,
TableNameBuilder.OFFLINE.tableNameWithType(tableName),
+ TableNameBuilder.REALTIME.tableNameWithType(tableName));
+ QuotaEnforcementMode result = null;
+ for (String entityName : entityNames) {
+ if (!_rateLimiterMap.containsKey(entityName)) {
+ continue;
+ }
+ QuotaEnforcementMode mode =
_enforcementModeOverrideMap.getOrDefault(entityName,
QuotaEnforcementMode.ENFORCED);
+ if (result == null || mode.ordinal() > result.ordinal()) {
Review Comment:
The "strictest wins" logic leans on `ordinal()` ordering, and the contract
for that lives on the SPI enum in another module. It's documented there, but a
future reorder/insert would break this silently with no compile-time or test
signal. A tiny test asserting `DISABLED < SHADOW < ENFORCED` (or an explicit
strictness mapping) would make the coupling safe.
##########
pinot-spi/src/main/java/org/apache/pinot/spi/config/table/QuotaEnforcementMode.java:
##########
@@ -0,0 +1,31 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.spi.config.table;
+
+/// Controls how a configured table quota (e.g. storage, query QPS) is
enforced. Declared in increasing order of
+/// strictness; code that has to pick one of several modes relies on the
ordinal.
Review Comment:
Good that the ordinal dependency is called out here. Since the actual
consumer is in pinot-broker (`getQueryQuotaEnforcementMode`), consider a guard
test so the two don't drift — serialization is name-based so reordering is safe
for persistence, but the strictness comparison would break quietly.
##########
pinot-spi/src/main/java/org/apache/pinot/spi/config/table/QuotaConfig.java:
##########
@@ -42,9 +42,27 @@ public class QuotaConfig extends BaseJsonConfig {
private transient final long _storageInBytes;
private transient final double _maxQPS;
+ // Optional per-table enforcement modes. Null means ENFORCED. They are
omitted from the serialized form when null,
+ // so configs that do not use them are unchanged and stay readable by
binaries that predate these fields.
+ @JsonPropertyDescription("How the storage quota is enforced: DISABLED,
SHADOW (breaches are metered and logged but "
+ + "not acted on) or ENFORCED. Defaults to ENFORCED")
+ private final QuotaEnforcementMode _storageQuotaEnforcementMode;
+
+ @JsonPropertyDescription("How the query QPS quota is enforced: DISABLED,
SHADOW (breaches are metered and logged "
+ + "but queries are not rejected) or ENFORCED. Defaults to ENFORCED")
+ private final QuotaEnforcementMode _queryQuotaEnforcementMode;
+
+ public QuotaConfig(@Nullable String storage, @Nullable String
maxQueriesPerSecond) {
+ this(storage, maxQueriesPerSecond, null, null);
Review Comment:
Heads up: this 2-arg constructor now drops the two enforcement modes
(delegates with `null, null`). That's fine here, but
`TableConfigUtils.ensureStorageQuotaConstraints()` reconstructs dimension-table
quota configs through this constructor and so silently loses a configured mode
— see the MAJOR note in the review summary. Worth a quick check of any other
call site that rebuilds a QuotaConfig from an existing one.
--
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]
