shounakmk219 commented on code in PR #13544:
URL: https://github.com/apache/pinot/pull/13544#discussion_r1677931484


##########
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:
   You are absolutely right. 
   Right now there is no defined relation between database and broker tenant 
which complicates the per broker rate calculation. We can apply different 
strategies here like get all the broker tenants under a database and only 
distribute quota among those brokers. But again what if tenant A get 90% of 
queries and tenant B tables get 10% of queries but tables tagged to tenant B 
are part of a bigger tenant so have more number of brokers tagged to them 
leading to less effective quota for tenant A brokers, we still end up giving 
less quota to the brokers with majority query load.
   Point being there is no right solution here. Based on how the user is 
leveraging the database concept with tenants, different strategies makes sense.
   The current strategy which equally distributes the quota equally across all 
brokers works well for a setup which has just 1 tenant (this would be the case 
most of the time). Starting with this for now and we can have a follow-up item 
to accommodate different quota distribution strategies as the database feature 
gets more used and new requirements come up.
   



-- 
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

Reply via email to