vrajat commented on code in PR #15839:
URL: https://github.com/apache/pinot/pull/15839#discussion_r2102908986
##########
pinot-broker/src/main/java/org/apache/pinot/broker/queryquota/HelixExternalViewBasedQueryQuotaManager.java:
##########
@@ -175,6 +176,20 @@ public void processClusterChange(HelixConstants.ChangeType
changeType) {
}
}
+ public void initOrUpdateLogicalTableQueryQuota(String logicalTableName) {
+ LogicalTableConfig logicalTableConfig =
ZKMetadataProvider.getLogicalTableConfig(_propertyStore, logicalTableName);
Review Comment:
Interesting - is this called in the query path as well ? I understand that
the same call is used in `initOrUpdateTableQueryQuota`
##########
pinot-broker/src/main/java/org/apache/pinot/broker/queryquota/HelixExternalViewBasedQueryQuotaManager.java:
##########
@@ -773,10 +796,14 @@ public void
processQueryRateLimitingExternalViewChange(ExternalView currentBroke
int onlineBrokerCount = otherOnlineBrokerCount + 1;
// Get stat from property store
- String tableConfigPath = constructTableConfigPath(tableNameWithType);
- Stat stat = _propertyStore.getStat(tableConfigPath,
AccessOption.PERSISTENT);
+ String physicalOrLogicalTableConfigPath =
+ ZKMetadataProvider.isTableConfigExists(_propertyStore,
physicalOrLogicalTableName)
Review Comment:
Is this in the query path ?
##########
pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/BaseBrokerRequestHandler.java:
##########
@@ -261,11 +261,18 @@ protected boolean hasExceededQPSQuota(@Nullable String
database, Set<String> tab
requestContext.setErrorCode(QueryErrorCode.TOO_MANY_REQUESTS);
return true;
}
- for (String tableName : tableNames) {
- if (!_queryQuotaManager.acquire(tableName)) {
- LOGGER.warn("Request {}: query exceeds quota for table: {}",
requestContext.getRequestId(), tableName);
+ for (String physicalOrLogicalTableName : tableNames) {
+ boolean acquired;
+ if (_tableCache.isLogicalTable(physicalOrLogicalTableName)) {
+ acquired =
_queryQuotaManager.acquireLogicalTable(physicalOrLogicalTableName);
+ } else {
+ acquired = _queryQuotaManager.acquire(physicalOrLogicalTableName);
+ }
+ if (!acquired) {
+ LOGGER.warn("Request {}: query exceeds quota for table: {}",
+ requestContext.getRequestId(), physicalOrLogicalTableName);
requestContext.setErrorCode(QueryErrorCode.TOO_MANY_REQUESTS);
- String rawTableName = TableNameBuilder.extractRawTableName(tableName);
+ String rawTableName =
TableNameBuilder.extractRawTableName(physicalOrLogicalTableName);
Review Comment:
nit: Can you move this into the else block ? Lets not call extract on a
logical table name even though its a no-op for the same of clean code.
--
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]
