xiangfu0 commented on code in PR #19838:
URL: https://github.com/apache/pinot/pull/19838#discussion_r4289844061
##########
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:
Small behavior change worth confirming is intentional: the logical-table
path now emits `QUERY_QUOTA_EXCEEDED` on a breach (via
`isQpsQuotaBreachEnforced`), whereas before this meter was only emitted for
physical tables. It's arguably a fix, but it's new metering for logical tables,
so flagging it so nobody is surprised by the new series.
##########
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:
Nit: `getEnforcementMode(tableConfig)` is now called twice per upload (once
inside `isSegmentStorageWithinQuota` and again here). It's cheap (just reads
config), so not worth restructuring, but you could have
`isSegmentStorageWithinQuota` return the resolved mode if you want to avoid the
second lookup.
##########
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:
Minor: a table can live in `_rateLimiterMap` with a null rate limiter (no
QPS quota configured), and `containsKey` counts it here. That's harmless today
because the mode is only consulted after `acquire()` reports a breach, and a
null-limiter table can't breach, so it never influences the strictest-mode
result. Just noting the subtlety in case the resolution gets reused elsewhere
later.
##########
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:
Nit: `getEnforcementMode(tableConfig)` is now called twice per upload (once
inside `isSegmentStorageWithinQuota` and again here). It's cheap (just reads
config), so not worth restructuring, but you could have
`isSegmentStorageWithinQuota` return the resolved mode if you want to avoid the
second lookup.
##########
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:
Minor: a table can live in `_rateLimiterMap` with a null rate limiter (no
QPS quota configured), and `containsKey` counts it here. That's harmless today
because the mode is only consulted after `acquire()` reports a breach, and a
null-limiter table can't breach, so it never influences the strictest-mode
result. Just noting the subtlety in case the resolution gets reused elsewhere
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:
Small behavior change worth confirming is intentional: the logical-table
path now emits `QUERY_QUOTA_EXCEEDED` on a breach (via
`isQpsQuotaBreachEnforced`), whereas before this meter was only emitted for
physical tables. It's arguably a fix, but it's new metering for logical tables,
so flagging it so nobody is surprised by the new series.
--
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]
