gortiz commented on code in PR #14226:
URL: https://github.com/apache/pinot/pull/14226#discussion_r1808372131
##########
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:
My comment wasn't related to the idea of modifying ITests. My comment is
related to the fact that this PR does two different things:
1. Adds application level query quota
2. Adds a new way to test that substitutes Thread.sleep with rate limiters
My ask is to do not add the latter in this PR.
--
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]
