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 2022/04/12 12:34:17 UTC
[GitHub] [pulsar] AnonHxy opened a new pull request, #15132: [broker]Tidy up update subscribeRateLimiter
AnonHxy opened a new pull request, #15132:
URL: https://github.com/apache/pulsar/pull/15132
### Motivation
Tidy up code to update subscribe rate-limiter
### Modifications
Use `updateSubscribeRateLimiter` to update subscribe rate-limiter
### Verifying this change
- [x] Make sure that the change passes the CI checks.
### Does this pull request potentially affect one of the following parts:
- Dependencies (does it add or upgrade a dependency): (no)
- The public API: (no)
- The schema: (no)
- The default values of configurations: (no)
- The wire protocol: (no)
- The rest endpoints: (no)
- The admin cli options: ( no)
- Anything that affects deployment: (no )
### Documentation
- [x] `no-need-doc`
(Please explain why)
--
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] lhotari commented on a diff in pull request #15132: [broker]Tidy up update subscribeRateLimiter
Posted by GitBox <gi...@apache.org>.
lhotari commented on code in PR #15132:
URL: https://github.com/apache/pulsar/pull/15132#discussion_r857160555
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentTopic.java:
##########
@@ -447,10 +442,12 @@ public void publishMessage(ByteBuf headersAndPayload, PublishContext publishCont
public void updateSubscribeRateLimiter() {
SubscribeRate subscribeRate = this.getSubscribeRate();
- if (isSubscribeRateEnabled(subscribeRate)) {
- subscribeRateLimiter = Optional.of(subscribeRateLimiter.orElse(new SubscribeRateLimiter(this)));
- } else {
- subscribeRateLimiter = Optional.empty();
+ synchronized (subscribeRateLimiter) {
Review Comment:
remove `synchronized (subscribeRateLimiter) {`. That is invalid as I explained previously. Making the field subscribeRateLimiter volatile is the correct solution.
--
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] HQebupt commented on pull request #15132: [broker]Tidy up update subscribeRateLimiter
Posted by GitBox <gi...@apache.org>.
HQebupt commented on PR #15132:
URL: https://github.com/apache/pulsar/pull/15132#issuecomment-1096735221
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] AnonHxy commented on pull request #15132: [broker]Tidy up update subscribeRateLimiter
Posted by GitBox <gi...@apache.org>.
AnonHxy commented on PR #15132:
URL: https://github.com/apache/pulsar/pull/15132#issuecomment-1100166353
/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] Jason918 commented on pull request #15132: [broker]Tidy up update subscribeRateLimiter
Posted by GitBox <gi...@apache.org>.
Jason918 commented on PR #15132:
URL: https://github.com/apache/pulsar/pull/15132#issuecomment-1100850709
@lhotari PTAL
--
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] AnonHxy commented on pull request #15132: [broker]Tidy up update subscribeRateLimiter
Posted by GitBox <gi...@apache.org>.
AnonHxy commented on PR #15132:
URL: https://github.com/apache/pulsar/pull/15132#issuecomment-1108190889
/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] AnonHxy commented on pull request #15132: [broker]Tidy up update subscribeRateLimiter
Posted by GitBox <gi...@apache.org>.
AnonHxy commented on PR #15132:
URL: https://github.com/apache/pulsar/pull/15132#issuecomment-1108636826
I find there is a race condition about `subscribeRateLimiter`. So I will close there PR and think a better solution. Thanks for all. @HQebupt @Jason918 @lhotari @codelipenghui
--
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] AnonHxy commented on pull request #15132: [broker]Tidy up update subscribeRateLimiter
Posted by GitBox <gi...@apache.org>.
AnonHxy commented on PR #15132:
URL: https://github.com/apache/pulsar/pull/15132#issuecomment-1108325758
/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] AnonHxy commented on pull request #15132: [broker]Tidy up update subscribeRateLimiter
Posted by GitBox <gi...@apache.org>.
AnonHxy commented on PR #15132:
URL: https://github.com/apache/pulsar/pull/15132#issuecomment-1108112405
/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] AnonHxy commented on a diff in pull request #15132: [broker]Tidy up update subscribeRateLimiter
Posted by GitBox <gi...@apache.org>.
AnonHxy commented on code in PR #15132:
URL: https://github.com/apache/pulsar/pull/15132#discussion_r857223487
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentTopic.java:
##########
@@ -447,10 +442,12 @@ public void publishMessage(ByteBuf headersAndPayload, PublishContext publishCont
public void updateSubscribeRateLimiter() {
SubscribeRate subscribeRate = this.getSubscribeRate();
- if (isSubscribeRateEnabled(subscribeRate)) {
- subscribeRateLimiter = Optional.of(subscribeRateLimiter.orElse(new SubscribeRateLimiter(this)));
- } else {
- subscribeRateLimiter = Optional.empty();
+ synchronized (subscribeRateLimiter) {
Review Comment:
OK. PTAL @lhotari :)
--
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] AnonHxy closed pull request #15132: [broker]Tidy up update subscribeRateLimiter
Posted by GitBox <gi...@apache.org>.
AnonHxy closed pull request #15132: [broker]Tidy up update subscribeRateLimiter
URL: https://github.com/apache/pulsar/pull/15132
--
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] lhotari commented on a diff in pull request #15132: [broker]Tidy up update subscribeRateLimiter
Posted by GitBox <gi...@apache.org>.
lhotari commented on code in PR #15132:
URL: https://github.com/apache/pulsar/pull/15132#discussion_r850094656
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentTopic.java:
##########
@@ -447,10 +442,12 @@ public void publishMessage(ByteBuf headersAndPayload, PublishContext publishCont
public void updateSubscribeRateLimiter() {
SubscribeRate subscribeRate = this.getSubscribeRate();
- if (isSubscribeRateEnabled(subscribeRate)) {
- subscribeRateLimiter = Optional.of(subscribeRateLimiter.orElse(new SubscribeRateLimiter(this)));
- } else {
- subscribeRateLimiter = Optional.empty();
+ synchronized (subscribeRateLimiter) {
Review Comment:
`synchronized(subscribeRateLimiter)` seems wrong. I can now see that this came from the original code. This should be fixed.
One possible fix is to make the `subscribeRateLimiter` field a `volatile` field.
--
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] Jason918 commented on pull request #15132: [broker]Tidy up update subscribeRateLimiter
Posted by GitBox <gi...@apache.org>.
Jason918 commented on PR #15132:
URL: https://github.com/apache/pulsar/pull/15132#issuecomment-1100513474
/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] AnonHxy commented on pull request #15132: [broker]Tidy up update subscribeRateLimiter
Posted by GitBox <gi...@apache.org>.
AnonHxy commented on PR #15132:
URL: https://github.com/apache/pulsar/pull/15132#issuecomment-1104667995
@lhotari @codelipenghui PTAL
--
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] AnonHxy commented on a diff in pull request #15132: [broker]Tidy up update subscribeRateLimiter
Posted by GitBox <gi...@apache.org>.
AnonHxy commented on code in PR #15132:
URL: https://github.com/apache/pulsar/pull/15132#discussion_r851228909
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentTopic.java:
##########
@@ -447,10 +442,12 @@ public void publishMessage(ByteBuf headersAndPayload, PublishContext publishCont
public void updateSubscribeRateLimiter() {
SubscribeRate subscribeRate = this.getSubscribeRate();
- if (isSubscribeRateEnabled(subscribeRate)) {
- subscribeRateLimiter = Optional.of(subscribeRateLimiter.orElse(new SubscribeRateLimiter(this)));
- } else {
- subscribeRateLimiter = Optional.empty();
+ synchronized (subscribeRateLimiter) {
Review Comment:
Thanks a lot. I have updated it. PTAL @lhotari @Jason918
--
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