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/05/10 06:11:12 UTC
[GitHub] [pulsar] AnonHxy opened a new pull request, #15520: [fix][broker]Close publishLimiter when disable it
AnonHxy opened a new pull request, #15520:
URL: https://github.com/apache/pulsar/pull/15520
### Motivation
Close `topicPublishRateLimiter` when it is disabled to prevent resource leakages
### Modifications
Close `topicPublishRateLimiter` when it change from enable to disable
### Verifying this change
- [x] Make sure that the change passes the CI checks.
- [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] Technoboy- commented on a diff in pull request #15520: [fix][broker]Close publishLimiter when disable it
Posted by GitBox <gi...@apache.org>.
Technoboy- commented on code in PR #15520:
URL: https://github.com/apache/pulsar/pull/15520#discussion_r870052674
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/AbstractTopic.java:
##########
@@ -1166,6 +1166,14 @@ public void updatePublishDispatcher() {
}
} else {
log.info("Disabling publish throttling for {}", this.topic);
+ if (topicPublishRateLimiter != null
+ && topicPublishRateLimiter != PublishRateLimiter.DISABLED_RATE_LIMITER) {
Review Comment:
I have override the `close` method by https://github.com/apache/pulsar/pull/15529
So you need to rebase and no need to try/catch.
--
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 #15520: [fix][broker]Close publishLimiter when disable it
Posted by GitBox <gi...@apache.org>.
AnonHxy commented on PR #15520:
URL: https://github.com/apache/pulsar/pull/15520#issuecomment-1123648133
/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 #15520: [fix][broker]Close publishLimiter when disable it
Posted by GitBox <gi...@apache.org>.
AnonHxy commented on PR #15520:
URL: https://github.com/apache/pulsar/pull/15520#issuecomment-1122085396
/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 #15520: [fix][broker]Close publishLimiter when disable it
Posted by GitBox <gi...@apache.org>.
AnonHxy commented on code in PR #15520:
URL: https://github.com/apache/pulsar/pull/15520#discussion_r869914327
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/AbstractTopic.java:
##########
@@ -1166,6 +1166,14 @@ public void updatePublishDispatcher() {
}
} else {
log.info("Disabling publish throttling for {}", this.topic);
+ if (topicPublishRateLimiter != null
+ && topicPublishRateLimiter != PublishRateLimiter.DISABLED_RATE_LIMITER) {
Review Comment:
OK, PTAL again @Technoboy-
--
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] Technoboy- commented on pull request #15520: [fix][broker]Close publishLimiter when disable it
Posted by GitBox <gi...@apache.org>.
Technoboy- commented on PR #15520:
URL: https://github.com/apache/pulsar/pull/15520#issuecomment-1122399351
Hi @lhotari
#10384 has defined `PublishRateLimiter` to extend `AutoCloseable` with `close` signs exception, and cause to catch the exception when updated. I think we should override the method in this interface to avoid throwing exceptions.
--
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] Technoboy- commented on a diff in pull request #15520: [fix][broker]Close publishLimiter when disable it
Posted by GitBox <gi...@apache.org>.
Technoboy- commented on code in PR #15520:
URL: https://github.com/apache/pulsar/pull/15520#discussion_r869236445
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/AbstractTopic.java:
##########
@@ -1166,6 +1166,14 @@ public void updatePublishDispatcher() {
}
} else {
log.info("Disabling publish throttling for {}", this.topic);
+ if (topicPublishRateLimiter != null
+ && topicPublishRateLimiter != PublishRateLimiter.DISABLED_RATE_LIMITER) {
Review Comment:
I think it's no need to test `topicPublishRateLimiter != PublishRateLimiter.DISABLED_RATE_LIMITER`
--
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] Technoboy- merged pull request #15520: [fix][broker]Close publishLimiter when disable it
Posted by GitBox <gi...@apache.org>.
Technoboy- merged PR #15520:
URL: https://github.com/apache/pulsar/pull/15520
--
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