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/04/26 13:26:43 UTC
[GitHub] [pulsar] lhotari opened a new pull request #10384: [Broker] Fix various issues when using preciseTopicPublishRateLimiterEnable=true
lhotari opened a new pull request #10384:
URL: https://github.com/apache/pulsar/pull/10384
### Motivation
When using `preciseTopicPublishRateLimiterEnable=true` (introduced by #7078) setting for rate limiting, there are various issues:
- updating the limits doesn't set either boundary when changing the limits from a bounded limit to unbounded.
- each topic will create a scheduler thread for each limiter instance
- each topic will never release the scheduler thread when the topic gets unloaded / closed
### Modifications
- Fix updating of the limits by cleaning up the previous limiter instances before creating new limiter instances
- Use `brokerService.pulsar().getExecutor()` as the scheduler for the rate limiter instances
- Add resource cleanup hooks for topic closing (unload)
### Open issue
The existing code has a difference in passing the `rateLimitFunction`:
https://github.com/apache/pulsar/blob/69a173a82c89893f54dbe5b6f422249f66ea5418/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/PrecisPublishLimiter.java#L80-L86
It's passed to the `topicPublishRateLimiterOnMessage`, but not to `topicPublishRateLimiterOnByte` . It is unclear whether this is intentional.
The `rateLimitFunction` is `() -> this.enableCnxAutoRead()`
https://github.com/apache/pulsar/blob/69a173a82c89893f54dbe5b6f422249f66ea5418/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/AbstractTopic.java#L913
(This also raises a question whether rate limiting works consistently when multiple topics share the same connection.)
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] lhotari commented on pull request #10384: [Broker] Fix various issues when using preciseTopicPublishRateLimiterEnable=true
Posted by GitBox <gi...@apache.org>.
lhotari commented on pull request #10384:
URL: https://github.com/apache/pulsar/pull/10384#issuecomment-830025917
@aloyszhang @codelipenghui @merlimat @eolivelli I'd appreciate your feedback on this PR Please review.
@aloyszhang
I'd also like to get some help in resolving the open issue described in the PR description. Is it intentional that `rateLimitFunction` isn't called for `topicPublishRateLimiterOnByte`?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] eolivelli commented on pull request #10384: [Broker] Fix various issues when using preciseTopicPublishRateLimiterEnable=true
Posted by GitBox <gi...@apache.org>.
eolivelli commented on pull request #10384:
URL: https://github.com/apache/pulsar/pull/10384#issuecomment-859353736
@aloyszhang @codelipenghui @merlimat 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] bharanic-dev commented on pull request #10384: [Broker] Fix set-publish-rate when using preciseTopicPublishRateLimiterEnable=true
Posted by GitBox <gi...@apache.org>.
bharanic-dev commented on pull request #10384:
URL: https://github.com/apache/pulsar/pull/10384#issuecomment-891977137
> > * at the time of configuration change, if the connection was in a state where the 'read is disabled' (because of a previous rate limit), should the RateLimiter call autoReadResetFunction() in it's 'close' method? Otherwise, would the connection deadlock and forever remain in 'read disable' state?
>
> @bharanic-dev Thanks for the feedback. I have added the call to the close method.
>
> > * for PIP-82, one rate limiter will be shared by multiple topics. So, when a topic is unloaded, the ratelimiter should not be closed unless it was the last topic that was using it. So, instead of making 'PublishRateLimiter' 'Autoclosable', can you please consider adding a 'detach' method that can be called when the topic is getting unloaded?
>
> @bharanic-dev I think it's better to handle this with composition instead of making a single class handle different use cases. With composition it becomes easy. You can have a wrapper that takes a PublishRateLimiter and handles the life cycle with reference counting. One possible solution is to use the org.apache.bookkeeper.mledger.util.AbstractCASReferenceCounted class as a base class for this kind of wrapper.
Thank you for taking care of this. 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] aloyszhang edited a comment on pull request #10384: [Broker] Fix various issues when using preciseTopicPublishRateLimiterEnable=true
Posted by GitBox <gi...@apache.org>.
aloyszhang edited a comment on pull request #10384:
URL: https://github.com/apache/pulsar/pull/10384#issuecomment-842292133
> It feels that the 1 second rate limiting interval would have to be shared between the 2 different rate limiters to fix the inconsistencies when both limits are set.
Yes, inconsistent between `publishThrottlingRateInMsg` and `publishThrottlingRateInByte` may happend.
Totally agree with that `publishThrottlingRateInMsg` and `publishThrottlingRateInByte` should share one same `rateTime ` and refresh ticket at the same time.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] aloyszhang edited a comment on pull request #10384: [Broker] Fix various issues when using preciseTopicPublishRateLimiterEnable=true
Posted by GitBox <gi...@apache.org>.
aloyszhang edited a comment on pull request #10384:
URL: https://github.com/apache/pulsar/pull/10384#issuecomment-842066557
> I'd also like to get some help in resolving the open issue described in the PR description. Is it intentional that `rateLimitFunction` isn't called for `topicPublishRateLimiterOnByte`?
Sorry for the late.
If we passed `rateLimitFunction` to both `topicPublishRateLimiterOnMessage` and `topicPublishRateLimiterOnByte`, the `() -> this.enableCnxAutoRead()` will be called twice each `rateTime`
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] lhotari commented on a change in pull request #10384: [Broker] Fix various issues when using preciseTopicPublishRateLimiterEnable=true
Posted by GitBox <gi...@apache.org>.
lhotari commented on a change in pull request #10384:
URL: https://github.com/apache/pulsar/pull/10384#discussion_r632344627
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/service/nonpersistent/NonPersistentTopic.java
##########
@@ -425,6 +425,13 @@ public void removeProducer(Producer producer) {
replicators.forEach((cluster, replicator) -> futures.add(replicator.disconnect()));
producers.values().forEach(producer -> futures.add(producer.disconnect()));
+ if (topicPublishRateLimiter instanceof AutoCloseable) {
Review comment:
yes, I guess it makes sense in this case since PublishRateLimiter is an internal interface and it's fine to change it.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] lhotari commented on pull request #10384: [Broker] Fix various issues when using preciseTopicPublishRateLimiterEnable=true
Posted by GitBox <gi...@apache.org>.
lhotari commented on pull request #10384:
URL: https://github.com/apache/pulsar/pull/10384#issuecomment-842092685
> If we passed `rateLimitFunction` to both `topicPublishRateLimiterOnMessage` and `topicPublishRateLimiterOnByte`, the `() -> this.enableCnxAutoRead()` will be called twice each `rateTime`
@aloyszhang Is it so that it should be called in both cases, but it's some kind of optimization to skip calling it for the other case?
If a possible duplicate call is the issue, is that something that should be fixed?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] aloyszhang edited a comment on pull request #10384: [Broker] Fix various issues when using preciseTopicPublishRateLimiterEnable=true
Posted by GitBox <gi...@apache.org>.
aloyszhang edited a comment on pull request #10384:
URL: https://github.com/apache/pulsar/pull/10384#issuecomment-842066557
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] lhotari commented on a change in pull request #10384: [Broker] Fix various issues when using preciseTopicPublishRateLimiterEnable=true
Posted by GitBox <gi...@apache.org>.
lhotari commented on a change in pull request #10384:
URL: https://github.com/apache/pulsar/pull/10384#discussion_r632344868
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/service/PrecisPublishLimiter.java
##########
@@ -71,31 +80,63 @@ public void update(Policies policies, String clusterName) {
: null;
this.update(maxPublishRate);
}
+
public void update(PublishRate maxPublishRate) {
- if (maxPublishRate != null
Review comment:
I think that would make the logic a bit complicated without much gains.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] aloyszhang commented on pull request #10384: [Broker] Fix various issues when using preciseTopicPublishRateLimiterEnable=true
Posted by GitBox <gi...@apache.org>.
aloyszhang commented on pull request #10384:
URL: https://github.com/apache/pulsar/pull/10384#issuecomment-842066557
> I'd also like to get some help in resolving the open issue described in the PR description. Is it intentional that `rateLimitFunction` isn't called for `topicPublishRateLimiterOnByte`?
Sorry for the late. This is not a intentional design. I think `rateLimitFunction` should also be called from `topicPublishRateLimiterOnByte`.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] lhotari commented on pull request #10384: [Broker] Fix various issues when using preciseTopicPublishRateLimiterEnable=true
Posted by GitBox <gi...@apache.org>.
lhotari commented on pull request #10384:
URL: https://github.com/apache/pulsar/pull/10384#issuecomment-826836760
@aloyszhang @codelipenghui @merlimat @eolivelli Please review. I'd also like to get some help in resolving the open issue described in the PR description.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] aloyszhang removed a comment on pull request #10384: [Broker] Fix various issues when using preciseTopicPublishRateLimiterEnable=true
Posted by GitBox <gi...@apache.org>.
aloyszhang removed a comment on pull request #10384:
URL: https://github.com/apache/pulsar/pull/10384#issuecomment-842130033
@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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] lhotari commented on a change in pull request #10384: [Broker] Fix various issues when using preciseTopicPublishRateLimiterEnable=true
Posted by GitBox <gi...@apache.org>.
lhotari commented on a change in pull request #10384:
URL: https://github.com/apache/pulsar/pull/10384#discussion_r633318908
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/service/nonpersistent/NonPersistentTopic.java
##########
@@ -425,6 +425,13 @@ public void removeProducer(Producer producer) {
replicators.forEach((cluster, replicator) -> futures.add(replicator.disconnect()));
producers.values().forEach(producer -> futures.add(producer.disconnect()));
+ if (topicPublishRateLimiter instanceof AutoCloseable) {
Review comment:
@eolivelli this is now resolved. 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] aloyszhang removed a comment on pull request #10384: [Broker] Fix various issues when using preciseTopicPublishRateLimiterEnable=true
Posted by GitBox <gi...@apache.org>.
aloyszhang removed a comment on pull request #10384:
URL: https://github.com/apache/pulsar/pull/10384#issuecomment-842130033
@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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] lhotari commented on pull request #10384: [Broker] Fix set-publish-rate when using preciseTopicPublishRateLimiterEnable=true
Posted by GitBox <gi...@apache.org>.
lhotari commented on pull request #10384:
URL: https://github.com/apache/pulsar/pull/10384#issuecomment-885639211
@sijie @aloyszhang @codelipenghui I have rebased the changes. Please review this PR and #11442 . There are other PRs #11352 and #11372 that depend on these changes so it would be good to get this PR processed asap. Thanks!
--
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 pull request #10384: [Broker] Fix various issues when using preciseTopicPublishRateLimiterEnable=true
Posted by GitBox <gi...@apache.org>.
lhotari commented on pull request #10384:
URL: https://github.com/apache/pulsar/pull/10384#issuecomment-842092685
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] lhotari commented on pull request #10384: [Broker] Fix various issues when using preciseTopicPublishRateLimiterEnable=true
Posted by GitBox <gi...@apache.org>.
lhotari commented on pull request #10384:
URL: https://github.com/apache/pulsar/pull/10384#issuecomment-885626912
#11442 was split from 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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] eolivelli commented on a change in pull request #10384: [Broker] Fix various issues when using preciseTopicPublishRateLimiterEnable=true
Posted by GitBox <gi...@apache.org>.
eolivelli commented on a change in pull request #10384:
URL: https://github.com/apache/pulsar/pull/10384#discussion_r623919273
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/service/PrecisPublishLimiter.java
##########
@@ -71,31 +80,63 @@ public void update(Policies policies, String clusterName) {
: null;
this.update(maxPublishRate);
}
+
public void update(PublishRate maxPublishRate) {
- if (maxPublishRate != null
Review comment:
can we exit in case that previous `maxPublishRate` is the same as the current one ?
this way we will save resources
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/service/nonpersistent/NonPersistentTopic.java
##########
@@ -425,6 +425,13 @@ public void removeProducer(Producer producer) {
replicators.forEach((cluster, replicator) -> futures.add(replicator.disconnect()));
producers.values().forEach(producer -> futures.add(producer.disconnect()));
+ if (topicPublishRateLimiter instanceof AutoCloseable) {
Review comment:
why can't we make every instance of topicPublishRateLimiter implement `AutoCloseable` ?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] bharanic-dev commented on pull request #10384: [Broker] Fix various issues when using preciseTopicPublishRateLimiterEnable=true
Posted by GitBox <gi...@apache.org>.
bharanic-dev commented on pull request #10384:
URL: https://github.com/apache/pulsar/pull/10384#issuecomment-867965727
@lhotari Excellent work. Thank you for taking care of all the issues. I observed some of the issues as I was studying the code to implement PIP-82 - great that your PR already addresses them all. I am still trying to learn the ropes here, so have a couple of dumb questions.
- at the time of configuration change, if the connection was in a state where the 'read is disabled' (because of a previous rate limit), should the RateLimiter call autoReadResetFunction() in it's 'close' method? Otherwise, would the connection deadlock and forever remain in 'read disable' state?
- for PIP-82, one rate limiter will be shared by multiple topics. So, when a topic is unloaded, the ratelimiter should not be closed unless it was the last topic that was using it. So, instead of making 'PublishRateLimiter' 'Autoclosable', can you please consider adding a 'detach' method that can be called when the topic is getting unloaded?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] lhotari commented on pull request #10384: [Broker] Fix set-publish-rate when using preciseTopicPublishRateLimiterEnable=true
Posted by GitBox <gi...@apache.org>.
lhotari commented on pull request #10384:
URL: https://github.com/apache/pulsar/pull/10384#issuecomment-889649336
@sijie @aloyszhang @codelipenghui Please review. The changes have been rebased.
#11352 and #11372 were merged before this PR. This PR is needed for fixing issues with set-publish-rate when using preciseTopicPublishRateLimiterEnable=true. /cc @ronfarkash @danielsinai
--
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 pull request #10384: [Broker] Fix set-publish-rate when using preciseTopicPublishRateLimiterEnable=true
Posted by GitBox <gi...@apache.org>.
lhotari commented on pull request #10384:
URL: https://github.com/apache/pulsar/pull/10384#issuecomment-885635317
> * at the time of configuration change, if the connection was in a state where the 'read is disabled' (because of a previous rate limit), should the RateLimiter call autoReadResetFunction() in it's 'close' method? Otherwise, would the connection deadlock and forever remain in 'read disable' state?
@bharanic-dev Thanks for the feedback. I have added the call to the close method.
> * for PIP-82, one rate limiter will be shared by multiple topics. So, when a topic is unloaded, the ratelimiter should not be closed unless it was the last topic that was using it. So, instead of making 'PublishRateLimiter' 'Autoclosable', can you please consider adding a 'detach' method that can be called when the topic is getting unloaded?
@bharanic-dev I think it's better to handle this with composition instead of making a single class handle different use cases. With composition it becomes easy. You can have a wrapper that takes a PublishRateLimiter and handles the life cycle with reference counting. One possible solution is to use the org.apache.bookkeeper.mledger.util.AbstractCASReferenceCounted class as a base class for this kind of wrapper.
--
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] sijie merged pull request #10384: [Broker] Fix set-publish-rate when using preciseTopicPublishRateLimiterEnable=true
Posted by GitBox <gi...@apache.org>.
sijie merged pull request #10384:
URL: https://github.com/apache/pulsar/pull/10384
--
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] eolivelli commented on pull request #10384: [Broker] Fix various issues when using preciseTopicPublishRateLimiterEnable=true
Posted by GitBox <gi...@apache.org>.
eolivelli commented on pull request #10384:
URL: https://github.com/apache/pulsar/pull/10384#issuecomment-841094967
@codelipenghui @merlimat @rdhabalia do you have any 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] aloyszhang edited a comment on pull request #10384: [Broker] Fix various issues when using preciseTopicPublishRateLimiterEnable=true
Posted by GitBox <gi...@apache.org>.
aloyszhang edited a comment on pull request #10384:
URL: https://github.com/apache/pulsar/pull/10384#issuecomment-842066557
> I'd also like to get some help in resolving the open issue described in the PR description. Is it intentional that `rateLimitFunction` isn't called for `topicPublishRateLimiterOnByte`?
Sorry for the late. If we passed `rateLimitFunction` to both `topicPublishRateLimiterOnMessage` and `topicPublishRateLimiterOnByte`, the enable `() -> this.enableCnxAutoRead()` twice each `rateTime`
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] aloyszhang edited a comment on pull request #10384: [Broker] Fix various issues when using preciseTopicPublishRateLimiterEnable=true
Posted by GitBox <gi...@apache.org>.
aloyszhang edited a comment on pull request #10384:
URL: https://github.com/apache/pulsar/pull/10384#issuecomment-842066557
> I'd also like to get some help in resolving the open issue described in the PR description. Is it intentional that `rateLimitFunction` isn't called for `topicPublishRateLimiterOnByte`?
Sorry for the late.
If we passed `rateLimitFunction` to both `topicPublishRateLimiterOnMessage` and `topicPublishRateLimiterOnByte`, the enable `() -> this.enableCnxAutoRead()` twice each `rateTime`
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] aloyszhang commented on pull request #10384: [Broker] Fix various issues when using preciseTopicPublishRateLimiterEnable=true
Posted by GitBox <gi...@apache.org>.
aloyszhang commented on pull request #10384:
URL: https://github.com/apache/pulsar/pull/10384#issuecomment-842292133
> It feels that the 1 second rate limiting interval would have to be shared between the 2 different rate limiters to fix the inconsistencies when both limits are set.
Yes, inconsistent between `publishThrottlingRateInMsg` and `publishThrottlingRateInByte` may happend.
Totally agree with that `publishThrottlingRateInMsg` and `publishThrottlingRateInByte` should share one same `rateTime ` and should refresh ticket at the same time.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] aloyszhang commented on pull request #10384: [Broker] Fix various issues when using preciseTopicPublishRateLimiterEnable=true
Posted by GitBox <gi...@apache.org>.
aloyszhang commented on pull request #10384:
URL: https://github.com/apache/pulsar/pull/10384#issuecomment-842129913
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] aloyszhang edited a comment on pull request #10384: [Broker] Fix various issues when using preciseTopicPublishRateLimiterEnable=true
Posted by GitBox <gi...@apache.org>.
aloyszhang edited a comment on pull request #10384:
URL: https://github.com/apache/pulsar/pull/10384#issuecomment-842292133
> It feels that the 1 second rate limiting interval would have to be shared between the 2 different rate limiters to fix the inconsistencies when both limits are set.
@lhotari Yes, inconsistent between `publishThrottlingRateInMsg` and `publishThrottlingRateInByte` may happend.
Totally agree with that `publishThrottlingRateInMsg` and `publishThrottlingRateInByte` should share one same `rateTime ` and refresh ticket at the same time.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] lhotari commented on pull request #10384: [Broker] Fix set-publish-rate when using preciseTopicPublishRateLimiterEnable=true
Posted by GitBox <gi...@apache.org>.
lhotari commented on pull request #10384:
URL: https://github.com/apache/pulsar/pull/10384#issuecomment-885632744
I have reduced the scope of this PR to cover the issues with `set-publish-rate`, including the resource leak with executors
--
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 change in pull request #10384: [Broker] Fix various issues when using preciseTopicPublishRateLimiterEnable=true
Posted by GitBox <gi...@apache.org>.
lhotari commented on a change in pull request #10384:
URL: https://github.com/apache/pulsar/pull/10384#discussion_r633318908
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/service/nonpersistent/NonPersistentTopic.java
##########
@@ -425,6 +425,13 @@ public void removeProducer(Producer producer) {
replicators.forEach((cluster, replicator) -> futures.add(replicator.disconnect()));
producers.values().forEach(producer -> futures.add(producer.disconnect()));
+ if (topicPublishRateLimiter instanceof AutoCloseable) {
Review comment:
@eolivelli this is now resolved. 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] lhotari commented on pull request #10384: [Broker] Fix various issues when using preciseTopicPublishRateLimiterEnable=true
Posted by GitBox <gi...@apache.org>.
lhotari commented on pull request #10384:
URL: https://github.com/apache/pulsar/pull/10384#issuecomment-842140533
> > Is it so that it should be called in both cases, but it's some kind of optimization to skip calling it for the other case? If a possible duplicate call is the issue, is that something that should be fixed?
>
> @lhotari Yes, since the `rateLimitFunction` is to enable channel autoread, called once has the same effect with called twice.
It seems that the rate limiting should be somehow combined when both limits `publishThrottlingRateInMsg` and `publishThrottlingRateInByte` are set. I would assume that it leads to inconsistent behavior when backpressure is released (enabling auto read) only in the `topicPublishRateLimiterOnMessage` rate limiter.
It feels that the 1 second rate limiting interval would have to be shared between the 2 different rate limiters to fix the inconsistencies when both limits are set. @aloyszhang WDYT?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] aloyszhang commented on pull request #10384: [Broker] Fix various issues when using preciseTopicPublishRateLimiterEnable=true
Posted by GitBox <gi...@apache.org>.
aloyszhang commented on pull request #10384:
URL: https://github.com/apache/pulsar/pull/10384#issuecomment-842066557
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org