Jackie-Jiang commented on code in PR #19838:
URL: https://github.com/apache/pinot/pull/19838#discussion_r4364754125
##########
pinot-broker/src/main/java/org/apache/pinot/broker/queryquota/HelixExternalViewBasedQueryQuotaManager.java:
##########
@@ -91,6 +93,7 @@ public class HelixExternalViewBasedQueryQuotaManager
implements ClusterChangeHan
private final String _instanceId;
private final AtomicInteger _lastKnownBrokerResourceVersion = new
AtomicInteger(-1);
private final Map<String, QueryQuotaEntity> _rateLimiterMap = new
ConcurrentHashMap<>();
+ private final Map<String, QuotaEnforcementMode> _enforcementModeOverrideMap
= new ConcurrentHashMap<>();
Review Comment:
Optional simplification: consider storing the effective enforcement mode on
`QueryQuotaEntity`, defaulting to `ENFORCED` and using a `volatile` field for
visibility to concurrent readers. This would remove the parallel
`_enforcementModeOverrideMap`, its update helper and test-only size accessor,
and duplicated cleanup at each table removal/reset path. The limiter and its
enforcement policy would then share one lifecycle.
##########
pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/BaseBrokerRequestHandler.java:
##########
@@ -311,17 +313,44 @@ protected boolean hasExceededQPSQuota(@Nullable String
database, Set<String> tab
return true;
}
for (String tableName : tableNames) {
- if (!_queryQuotaManager.acquire(tableName)) {
+ if (isTableQpsQuotaBreachEnforced(tableName)) {
LOGGER.warn("Request {}: query exceeds quota for table: {}",
requestContext.getRequestId(), tableName);
requestContext.setErrorCode(QueryErrorCode.TOO_MANY_REQUESTS);
- String rawTableName = TableNameBuilder.extractRawTableName(tableName);
- _brokerMetrics.addMeteredTableValue(rawTableName,
BrokerMeter.QUERY_QUOTA_EXCEEDED, 1);
return true;
}
}
return false;
}
+ /// Acquires the QPS quota of the given table (name with or without type
suffix) and returns true only if the quota
+ /// is breached and the table's enforcement mode requires rejecting the
query.
+ protected boolean isTableQpsQuotaBreachEnforced(String tableName) {
+ return isQpsQuotaBreachEnforced(tableName, _queryQuotaManager::acquire);
+ }
+
+ /// Same as [#isTableQpsQuotaBreachEnforced(String)] for a logical table.
+ protected boolean isLogicalTableQpsQuotaBreachEnforced(String
logicalTableName) {
+ return isQpsQuotaBreachEnforced(logicalTableName,
_queryQuotaManager::acquireLogicalTable);
+ }
+
+ private boolean isQpsQuotaBreachEnforced(String tableName, Predicate<String>
acquire) {
Review Comment:
Optional simplification: the two wrappers could call `acquire()` /
`acquireLogicalTable()` directly and share only the breach handling, removing
the `Predicate<String>` parameter and method-reference indirection. For example:
```java
return !_queryQuotaManager.acquire(tableName) &&
shouldEnforceQuotaBreach(tableName);
```
The shared helper would start with mode resolution. Short-circuiting
preserves the current behavior of looking up the mode only after acquisition
fails.
##########
pinot-controller/src/main/java/org/apache/pinot/controller/validation/RealtimeSegmentValidationManager.java:
##########
@@ -196,6 +197,16 @@ boolean shouldEnsureConsuming(String tableNameWithType) {
// Perform storage quota checks.
TableConfig tableConfig =
_pinotHelixResourceManager.getTableConfig(tableNameWithType);
boolean isQuotaExceeded =
_storageQuotaChecker.isTableStorageQuotaExceeded(tableConfig);
+ QuotaEnforcementMode enforcementMode =
_storageQuotaChecker.getEnforcementMode(tableConfig);
+ if (isQuotaExceeded && enforcementMode != QuotaEnforcementMode.ENFORCED) {
+ if (enforcementMode == QuotaEnforcementMode.SHADOW) {
+ // The breach meter is already emitted by StorageQuotaChecker
+ LOGGER.warn("Storage quota exceeded for table: {}, but enforcement
mode is SHADOW, not pausing consumption",
+ tableNameWithType);
+ }
+ // DISABLED or SHADOW: don't treat the breach as exceeded for
pause/resume purposes
+ isQuotaExceeded = false;
Review Comment:
Optional simplification: keep `isQuotaExceeded` as the raw breach result and
derive the enforcement decision separately:
```java
boolean shouldPauseForQuota = isQuotaExceeded && enforcementMode ==
QuotaEnforcementMode.ENFORCED;
```
Use `shouldPauseForQuota` for the pause-state comparison, transition and
return, while retaining the SHADOW log. This avoids changing the meaning of
`isQuotaExceeded` midway through the method and preserves breach metering in
the checker.
--
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]
