Jackie-Jiang commented on code in PR #14859:
URL: https://github.com/apache/pinot/pull/14859#discussion_r1931270796
##########
pinot-broker/src/main/java/org/apache/pinot/broker/queryquota/HelixExternalViewBasedQueryQuotaManager.java:
##########
@@ -130,9 +136,10 @@ private void initializeApplicationQpsQuotas() {
String appName = entry.getKey();
double appQpsQuota =
- entry.getValue() != null && entry.getValue() != -1.0d ?
entry.getValue() : _defaultQpsQuotaForApplication;
+ entry.getValue() != null && entry.getValue() >= MIN_APP_QUOTA ?
entry.getValue()
Review Comment:
Not introduced in this PR, but we might want to allow overriding default
quota to disable throttling
```suggestion
entry.getValue() != null ? entry.getValue() :
_defaultQpsQuotaForApplication;
```
##########
pinot-broker/src/main/java/org/apache/pinot/broker/queryquota/HelixExternalViewBasedQueryQuotaManager.java:
##########
@@ -74,6 +74,12 @@
* - broker added or removed from cluster
*/
public class HelixExternalViewBasedQueryQuotaManager implements
ClusterChangeHandler, QueryQuotaManager {
+
+ // Minimum 'working' value for app quota. If actual value is less than this
(e.g. 0.0), it is considered as disabled.
+ private static final double MIN_APP_QUOTA = Math.nextUp(0.0);
Review Comment:
I think the intention here is to treat `0` as disabled. It is not very
readable to have this minimum double, can we change the comparison (e.g. `<` to
`<=`) sign instead?
##########
pinot-broker/src/main/java/org/apache/pinot/broker/queryquota/HelixExternalViewBasedQueryQuotaManager.java:
##########
@@ -348,16 +355,38 @@ private synchronized void
createOrUpdateDatabaseRateLimiter(List<String> databas
}
public synchronized void createOrUpdateApplicationRateLimiter(String
applicationName) {
-
createOrUpdateApplicationRateLimiter(Collections.singletonList(applicationName));
+
createOrUpdateApplicationRateLimiter(Collections.singletonList(applicationName),
DISABLED_APP_QUOTA);
+ }
+
+ public synchronized void createOrUpdateApplicationRateLimiter(String
applicationName, double override) {
+
createOrUpdateApplicationRateLimiter(Collections.singletonList(applicationName),
override);
}
// Caller method need not worry about getting lock on
_applicationRateLimiterMap
// as this method will do idempotent updates to the application rate limiters
- private synchronized void createOrUpdateApplicationRateLimiter(List<String>
applicationNames) {
+ private synchronized void createOrUpdateApplicationRateLimiter(List<String>
applicationNames, double override) {
Review Comment:
Consider making override a `@Nullable`, and use `null` to represent not
override
##########
pinot-broker/src/main/java/org/apache/pinot/broker/queryquota/HelixExternalViewBasedQueryQuotaManager.java:
##########
@@ -130,9 +136,10 @@ private void initializeApplicationQpsQuotas() {
String appName = entry.getKey();
double appQpsQuota =
- entry.getValue() != null && entry.getValue() != -1.0d ?
entry.getValue() : _defaultQpsQuotaForApplication;
+ entry.getValue() != null && entry.getValue() >= MIN_APP_QUOTA ?
entry.getValue()
+ : _defaultQpsQuotaForApplication;
- if (appQpsQuota < 0) {
+ if (appQpsQuota < MIN_APP_QUOTA) {
Review Comment:
Same for other places
```suggestion
if (appQpsQuota <= 0) {
```
--
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
