vrajat commented on code in PR #13544:
URL: https://github.com/apache/pinot/pull/13544#discussion_r1679268404
##########
pinot-broker/src/main/java/org/apache/pinot/broker/queryquota/HelixExternalViewBasedQueryQuotaManager.java:
##########
@@ -46,21 +55,31 @@
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
+import static
org.apache.pinot.spi.utils.CommonConstants.Helix.DATABASE_QUERY_RATE_LIMIT;
+
/**
* This class is to support the qps quota feature.
- * It depends on the broker source change to update the dynamic rate limit,
- * which means it only gets updated when a new table added or a broker
restarted.
+ * It allows performing qps quota check at table level and database level
+ * For table level check it depends on the broker source change to update the
dynamic rate limit,
+ * which means it gets updated when a new table added or a broker restarted.
+ * For database level check it depends on the broker as well as cluster config
change to update the dynamic rate limit,
+ * which means it gets updated when the default query quota at cluster config
is updated or a broker restarted.
*/
public class HelixExternalViewBasedQueryQuotaManager implements
ClusterChangeHandler, QueryQuotaManager {
Review Comment:
Do you plan to add tests? I see that there is a corresponding test class.
##########
pinot-spi/src/main/java/org/apache/pinot/spi/config/table/QuotaConfig.java:
##########
@@ -35,7 +35,8 @@ public class QuotaConfig extends BaseJsonConfig {
private static final long INVALID_STORAGE_IN_BYTES = -1L;
private static final double INVALID_MAX_QPS = -1.0;
- @JsonPropertyDescription("Storage allocated for this table, e.g. \"10G\"")
+
+ @JsonPropertyDescription("Storage allocated for this table/database, e.g.
\"10G\"")
Review Comment:
Is storage quota implemented ?
##########
pinot-common/src/main/java/org/apache/pinot/common/utils/DatabaseUtils.java:
##########
@@ -27,6 +27,8 @@
import org.apache.pinot.spi.exception.DatabaseConflictException;
import org.apache.pinot.spi.utils.CommonConstants;
+import static org.apache.pinot.spi.utils.CommonConstants.DATABASE;
Review Comment:
Is static imports allowed? I remember checkstyle failing for me. I see that
github actions passed. Surprised.
##########
pinot-broker/src/main/java/org/apache/pinot/broker/queryquota/HelixExternalViewBasedQueryQuotaManager.java:
##########
@@ -230,6 +248,72 @@ private void createOrUpdateRateLimiter(String
tableNameWithType, ExternalView br
}
}
+ /**
+ * Updates the database rate limiter if it already exists. Will not create a
new database rate limiter.
+ * @param databaseName database name for which rate limiter needs to be
updated
+ */
+ public void updateDatabaseRateLimiter(String databaseName) {
+ if (!_databaseRateLimiterMap.containsKey(databaseName)) {
+ return;
+ }
+ createOrUpdateDatabaseRateLimiter(Collections.singletonList(databaseName));
+ }
+
+ private void createOrUpdateDatabaseRateLimiter(List<String> databaseNames) {
+ double databaseQpsQuota = _defaultQpsQuotaForDatabase;
+ ExternalView brokerResource = HelixHelper
+ .getExternalViewForResource(_helixManager.getClusterManagmentTool(),
_helixManager.getClusterName(),
+ CommonConstants.Helix.BROKER_RESOURCE_INSTANCE);
+ // Tables in database can span across broker tags as we don't maintain a
broker tag to database mapping as of now.
+ // Hence, we consider all online brokers for the rate distribution.
+ int onlineBrokers =
HelixHelper.getOnlineInstanceFromExternalView(brokerResource).size();
+ for (String databaseName : databaseNames) {
+ DatabaseConfig databaseConfig =
+
ZKMetadataProvider.getDatabaseConfig(_helixManager.getHelixPropertyStore(),
databaseName);
+ if (databaseConfig != null && databaseConfig.getQuotaConfig() != null
+ && databaseConfig.getQuotaConfig().getMaxQPS() != -1) {
+ databaseQpsQuota = databaseConfig.getQuotaConfig().getMaxQPS();
+ }
+ if (databaseQpsQuota < 0) {
+ buildEmptyOrResetRateLimiterInDatabaseQueryQuotaEntity(databaseName);
+ continue;
+ }
+ double perBrokerQpsQuota = databaseQpsQuota / onlineBrokers;
Review Comment:
Does table level quota also get divided among all brokers in a tenant?
--
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: commits-unsubscr...@pinot.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org
For additional commands, e-mail: commits-h...@pinot.apache.org
