Tarun-kishore commented on code in PR #19838:
URL: https://github.com/apache/pinot/pull/19838#discussion_r4290722766
##########
pinot-broker/src/main/java/org/apache/pinot/broker/queryquota/HelixExternalViewBasedQueryQuotaManager.java:
##########
@@ -46,6 +47,7 @@
import org.apache.pinot.common.utils.helix.HelixHelper;
import org.apache.pinot.spi.config.DatabaseConfig;
import org.apache.pinot.spi.config.table.QuotaConfig;
Review Comment:
Agreed, this is intentional. A table with a mode but no QPS quota stays in
`_rateLimiterMap` with a null limiter, and its mode is still reported. This is
covered by `testQueryQuotaEnforcementModeOverrideTracksTableConfig`.
It shouldn't cause any issues since the mode is only checked after
`acquire()` reports a breach, which can't happen with a null limiter. I've
added a comment explaining this at the check to avoid confusion if this logic
is reused later.
##########
pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/BaseSingleStageBrokerRequestHandler.java:
##########
@@ -536,7 +536,7 @@ protected BrokerResponse doHandleRequest(long requestId,
String query, SqlNodeAn
requestContext.setErrorCode(QueryErrorCode.TOO_MANY_REQUESTS);
return new BrokerResponseNative(QueryErrorCode.TOO_MANY_REQUESTS,
errorMessage);
}
- if (!_queryQuotaManager.acquireLogicalTable(tableName)) {
+ if (isLogicalTableQpsQuotaBreachEnforced(tableName)) {
Review Comment:
Yes, intentional. Before this change the logical-table path rejected with
429 but never emitted `QUERY_QUOTA_EXCEEDED`, which would have made SHADOW mode
invisible for logical tables. Both paths now go through the same
`isQpsQuotaBreachEnforced`, so physical and logical tables emit the same meter
in ENFORCED and SHADOW (and neither in DISABLED). The series is keyed by the
logical table name, as it is for physical tables by raw table name.
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/upload/SegmentValidationUtils.java:
##########
@@ -143,6 +144,12 @@ public static void checkStorageQuota(String segmentName,
long tarSegmentSizeInBy
tableConfig.getTableName()),
Response.Status.INTERNAL_SERVER_ERROR);
}
if (!response._isSegmentWithinQuota) {
+ QuotaEnforcementMode enforcementMode =
quotaChecker.getEnforcementMode(tableConfig);
Review Comment:
Right, it's just two cheap config reads. I'd prefer to keep
`isSegmentStorageWithinQuota`'s return type unchanged since other callers
depend on it, rather than change the API for such a small optimization. Let me
know if you see a reason to do it differently.
--
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]
