gortiz commented on code in PR #14226:
URL: https://github.com/apache/pinot/pull/14226#discussion_r1808290532


##########
pinot-broker/src/test/java/org/apache/pinot/broker/queryquota/HelixExternalViewBasedQueryQuotaManagerTest.java:
##########
@@ -531,25 +674,41 @@ private static ExternalView generateBrokerResource(String 
tableName) {
   private void runQueries()
       throws InterruptedException {
     runQueries(TABLE_MAX_QPS, false);
-    //increase the qps and some of the queries should be throttled.
-    runQueries(TABLE_MAX_QPS * 2, true);
+    // increase the qps and some of the queries should be throttled.
+    // keep in mind that permits are 'regenerated' on every call based on how 
much time elapsed since last one
+    // that means for 25 QPS we get new permit every 40 ms or 0.5 every 20 ms
+    // if we start with 25 permits at time t1 then if we want to exceed the 
qps in the next second  we've to do more
+    // double requests, because 25 will regenerate
+    runQueries(TABLE_MAX_QPS * 2 + 1, true);
+  }
+
+  private void runQueries(double qps, boolean shouldFail)
+      throws InterruptedException {
+    runQueries(qps, shouldFail, APP_NAME);
   }
 
   // try to keep the qps below 50 to ensure that the time lost between 2 query 
runs on top of the sleepMillis
   // is not comparable to sleepMillis, else the actual qps would end being lot 
lower than required qps
-  private void runQueries(double qps, boolean shouldFail)
+  private void runQueries(double qps, boolean shouldFail, String appName)
       throws InterruptedException {
     int failCount = 0;
     long sleepMillis = (long) (1000 / qps);
     for (int i = 0; i < qps; i++) {
+      if (!_queryQuotaManager.acquireApplication(appName)) {
+        failCount++;
+      }
       if 
(!_queryQuotaManager.acquireDatabase(CommonConstants.DEFAULT_DATABASE)) {
         failCount++;
       }
       if (!_queryQuotaManager.acquire(RAW_TABLE_NAME)) {
         failCount++;
       }
-      Thread.sleep(sleepMillis);

Review Comment:
   Any improvement here would be great, but I think they should be discussed in 
another PR, not here. It is not clear to me how this proposed system works and 
I don't think we should block the application level quota in order to discuss 
about the timesource method



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

Reply via email to