You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pulsar.apache.org by GitBox <gi...@apache.org> on 2021/09/03 16:00:27 UTC
[GitHub] [pulsar] bharanic-dev opened a new pull request #11916: [PIP-82] [pulsar-broker] Miscellaneous bug fixes in resource group rate limiter.
bharanic-dev opened a new pull request #11916:
URL: https://github.com/apache/pulsar/pull/11916
### Motivation
Fix a few small issues found during testing.
### Modifications
- publish rate limiter should not be set for dispatch (dispatch is not yet supported).
- if the calculated quota is 0, set it to 1 (value 0 is assumed to mean that the rate limiter is not enabled).
### Verifying this change
- [x] Make sure that the change passes the CI checks.
- [x] tested manually
This change is a trivial rework / code cleanup without any test coverage.
### Documentation
Check the box below and label this PR (if you have committer privilege).
- [x] no-need-doc
internal bug fixes.
--
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-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] bharanic-dev commented on pull request #11916: [PIP-82] [pulsar-broker] Miscellaneous bug fixes in resource group rate limiter.
Posted by GitBox <gi...@apache.org>.
bharanic-dev commented on pull request #11916:
URL: https://github.com/apache/pulsar/pull/11916#issuecomment-913192226
/pulsarbot run-failure-checks
--
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-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] kaushik-develop commented on a change in pull request #11916: [PIP-82] [pulsar-broker] Miscellaneous bug fixes in resource group rate limiter.
Posted by GitBox <gi...@apache.org>.
kaushik-develop commented on a change in pull request #11916:
URL: https://github.com/apache/pulsar/pull/11916#discussion_r702057120
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/resourcegroup/ResourceQuotaCalculatorImpl.java
##########
@@ -80,6 +74,12 @@ public long computeLocalQuota(long confUsage, long myUsage, long[] allUsages) th
float myUsageFraction = (float) myUsage / totalUsage;
float calculatedQuota = max(myUsage + residual * myUsageFraction, 0);
+ // If the calculated rate ends up being 0, set it to 0. rate value of 0 or less is assumed to mean that
Review comment:
It isn't clear why 0 is assumed to mean "no rate limiter is set", if negative values also connote the same thing. It is conceivable that a rate limiter could be set, and the rate would be set to 0, right?
--
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-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] ravi-vaidyanathan commented on pull request #11916: [PIP-82] [pulsar-broker] Miscellaneous bug fixes in resource group rate limiter.
Posted by GitBox <gi...@apache.org>.
ravi-vaidyanathan commented on pull request #11916:
URL: https://github.com/apache/pulsar/pull/11916#issuecomment-912831002
LGTM
--
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-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] kaushik-develop commented on a change in pull request #11916: [PIP-82] [pulsar-broker] Miscellaneous bug fixes in resource group rate limiter.
Posted by GitBox <gi...@apache.org>.
kaushik-develop commented on a change in pull request #11916:
URL: https://github.com/apache/pulsar/pull/11916#discussion_r702055889
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/resourcegroup/ResourceGroup.java
##########
@@ -341,7 +341,11 @@ protected BytesAndMessagesCount updateLocalQuota(ResourceGroupMonitoringClass mo
oldBMCount = monEntity.quotaForNextPeriod;
try {
monEntity.quotaForNextPeriod = newQuota;
- this.resourceGroupPublishLimiter.update(newQuota);
+ if (monClass == ResourceGroupMonitoringClass.Publish) {
+ this.resourceGroupPublishLimiter.update(newQuota);
+ } else {
Review comment:
Since we are early-quitting for anything except Publish (see line 329), I didn't see why this check for non-Publish is required again in line 346.
Small nit: in earlier code review, someone asked me to change the comparison to Publish.equals (instead of the direct "==" check). JFYI, in case this check here needs to remain for some reason.
--
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-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] bharanic-dev commented on pull request #11916: [PIP-82] [pulsar-broker] Miscellaneous bug fixes in resource group rate limiter.
Posted by GitBox <gi...@apache.org>.
bharanic-dev commented on pull request #11916:
URL: https://github.com/apache/pulsar/pull/11916#issuecomment-913250466
/pulsarbot run-failure-checks
--
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-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] bharanic-dev commented on pull request #11916: [PIP-82] [pulsar-broker] Miscellaneous bug fixes in resource group rate limiter.
Posted by GitBox <gi...@apache.org>.
bharanic-dev commented on pull request #11916:
URL: https://github.com/apache/pulsar/pull/11916#issuecomment-912866589
/pulsarbot run-failure-checks
--
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-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] kaushik-develop commented on a change in pull request #11916: [PIP-82] [pulsar-broker] Miscellaneous bug fixes in resource group rate limiter.
Posted by GitBox <gi...@apache.org>.
kaushik-develop commented on a change in pull request #11916:
URL: https://github.com/apache/pulsar/pull/11916#discussion_r702057120
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/resourcegroup/ResourceQuotaCalculatorImpl.java
##########
@@ -80,6 +74,12 @@ public long computeLocalQuota(long confUsage, long myUsage, long[] allUsages) th
float myUsageFraction = (float) myUsage / totalUsage;
float calculatedQuota = max(myUsage + residual * myUsageFraction, 0);
+ // If the calculated rate ends up being 0, set it to 0. rate value of 0 or less is assumed to mean that
Review comment:
It isn't clear why 0 is assumed to mean "no rate limiter is set", if negative values also connote the same thing. It is conceivable that a rate limiter could be set, with the "allowed rate" configured to 0, right?
--
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-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] bharanic-dev commented on a change in pull request #11916: [PIP-82] [pulsar-broker] Miscellaneous bug fixes in resource group rate limiter.
Posted by GitBox <gi...@apache.org>.
bharanic-dev commented on a change in pull request #11916:
URL: https://github.com/apache/pulsar/pull/11916#discussion_r702080171
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/resourcegroup/ResourceQuotaCalculatorImpl.java
##########
@@ -80,6 +74,12 @@ public long computeLocalQuota(long confUsage, long myUsage, long[] allUsages) th
float myUsageFraction = (float) myUsage / totalUsage;
float calculatedQuota = max(myUsage + residual * myUsageFraction, 0);
+ // If the calculated rate ends up being 0, set it to 0. rate value of 0 or less is assumed to mean that
Review comment:
@kaushik-develop else-where in the RateLimiter code, that is the convention that is currently followed. See the code here:
https://github.com/apache/pulsar/blob/a7bdc5e25778ada8c781e9b3f9cc4694e5fd3f58/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/PrecisPublishLimiter.java#L97
And I don't plan to change the precedence (as that will only cause more confusion).
--
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-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] bharanic-dev commented on pull request #11916: [PIP-82] [pulsar-broker] Miscellaneous bug fixes in resource group rate limiter.
Posted by GitBox <gi...@apache.org>.
bharanic-dev commented on pull request #11916:
URL: https://github.com/apache/pulsar/pull/11916#issuecomment-914414563
/pulsarbot run-failure-checks
--
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-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] cckellogg merged pull request #11916: [PIP-82] [pulsar-broker] Miscellaneous bug fixes in resource group rate limiter.
Posted by GitBox <gi...@apache.org>.
cckellogg merged pull request #11916:
URL: https://github.com/apache/pulsar/pull/11916
--
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-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] kaushik-develop commented on a change in pull request #11916: [PIP-82] [pulsar-broker] Miscellaneous bug fixes in resource group rate limiter.
Posted by GitBox <gi...@apache.org>.
kaushik-develop commented on a change in pull request #11916:
URL: https://github.com/apache/pulsar/pull/11916#discussion_r702055889
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/resourcegroup/ResourceGroup.java
##########
@@ -341,7 +341,11 @@ protected BytesAndMessagesCount updateLocalQuota(ResourceGroupMonitoringClass mo
oldBMCount = monEntity.quotaForNextPeriod;
try {
monEntity.quotaForNextPeriod = newQuota;
- this.resourceGroupPublishLimiter.update(newQuota);
+ if (monClass == ResourceGroupMonitoringClass.Publish) {
+ this.resourceGroupPublishLimiter.update(newQuota);
+ } else {
Review comment:
Since we are early-quitting for anything except Publish (see line 329), I didn't see why this check for non-Publish is required again in line 346.
--
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-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] bharanic-dev commented on pull request #11916: [PIP-82] [pulsar-broker] Miscellaneous bug fixes in resource group rate limiter.
Posted by GitBox <gi...@apache.org>.
bharanic-dev commented on pull request #11916:
URL: https://github.com/apache/pulsar/pull/11916#issuecomment-912852373
/pulsarbot run-failure-checks
--
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-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] bharanic-dev commented on a change in pull request #11916: [PIP-82] [pulsar-broker] Miscellaneous bug fixes in resource group rate limiter.
Posted by GitBox <gi...@apache.org>.
bharanic-dev commented on a change in pull request #11916:
URL: https://github.com/apache/pulsar/pull/11916#discussion_r702068431
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/resourcegroup/ResourceQuotaCalculatorImpl.java
##########
@@ -80,6 +74,12 @@ public long computeLocalQuota(long confUsage, long myUsage, long[] allUsages) th
float myUsageFraction = (float) myUsage / totalUsage;
float calculatedQuota = max(myUsage + residual * myUsageFraction, 0);
+ // If the calculated rate ends up being 0, set it to 0. rate value of 0 or less is assumed to mean that
Review comment:
thank you @ravi-vaidyanathan , will fix the comment.
--
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-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] ravi-vaidyanathan commented on a change in pull request #11916: [PIP-82] [pulsar-broker] Miscellaneous bug fixes in resource group rate limiter.
Posted by GitBox <gi...@apache.org>.
ravi-vaidyanathan commented on a change in pull request #11916:
URL: https://github.com/apache/pulsar/pull/11916#discussion_r702034920
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/resourcegroup/ResourceQuotaCalculatorImpl.java
##########
@@ -80,6 +74,12 @@ public long computeLocalQuota(long confUsage, long myUsage, long[] allUsages) th
float myUsageFraction = (float) myUsage / totalUsage;
float calculatedQuota = max(myUsage + residual * myUsageFraction, 0);
+ // If the calculated rate ends up being 0, set it to 0. rate value of 0 or less is assumed to mean that
Review comment:
should the comment say "set it to 1"
--
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-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] bharanic-dev commented on pull request #11916: [PIP-82] [pulsar-broker] Miscellaneous bug fixes in resource group rate limiter.
Posted by GitBox <gi...@apache.org>.
bharanic-dev commented on pull request #11916:
URL: https://github.com/apache/pulsar/pull/11916#issuecomment-912858681
/pulsarbot run-failure-checks
--
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-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] bharanic-dev commented on a change in pull request #11916: [PIP-82] [pulsar-broker] Miscellaneous bug fixes in resource group rate limiter.
Posted by GitBox <gi...@apache.org>.
bharanic-dev commented on a change in pull request #11916:
URL: https://github.com/apache/pulsar/pull/11916#discussion_r702070778
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/resourcegroup/ResourceGroup.java
##########
@@ -341,7 +341,11 @@ protected BytesAndMessagesCount updateLocalQuota(ResourceGroupMonitoringClass mo
oldBMCount = monEntity.quotaForNextPeriod;
try {
monEntity.quotaForNextPeriod = newQuota;
- this.resourceGroupPublishLimiter.update(newQuota);
+ if (monClass == ResourceGroupMonitoringClass.Publish) {
+ this.resourceGroupPublishLimiter.update(newQuota);
+ } else {
Review comment:
when I made this change in my local sandbox, I did not have the changes you committed in commit ID '60a569823293'. Now that the check is added in commit ID '60a569823293', this change is no longer required. I will update the code.
--
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-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org