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/25 16:42:52 UTC
[GitHub] [pulsar] AnonHxy opened a new pull request, #15789: [broker]tidy update subscriptions dispatcher rate-limiter
AnonHxy opened a new pull request, #15789:
URL: https://github.com/apache/pulsar/pull/15789
### Motivation
* When updating topic-level and namespace-level subscriptions dispatcher rate-limiter, the code is a little different but we can unify them as one method, such as `updateSubscriptionsDispatcherRateLimiter()`, which will make the code more clear
### Modifications
Create `updateSubscriptionsDispatcherRateLimiter()` to update subscriptions dispatcher rate-limiter on both topic-level and namespace-level
### Verifying this change
- [x] Make sure that the change passes the CI checks.
### Documentation
- [x] `doc`
--
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 #15789: [broker]tidy update subscriptions dispatcher rate-limiter
Posted by GitBox <gi...@apache.org>.
AnonHxy commented on PR #15789:
URL: https://github.com/apache/pulsar/pull/15789#issuecomment-1140283418
/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 #15789: [broker]tidy update subscriptions dispatcher rate-limiter
Posted by GitBox <gi...@apache.org>.
AnonHxy commented on code in PR #15789:
URL: https://github.com/apache/pulsar/pull/15789#discussion_r884085548
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentTopic.java:
##########
@@ -3066,6 +3036,19 @@ public void onUpdate(TopicPolicies policies) {
});
}
+ private CompletableFuture<Void> updateSubscriptionsDispatcherRateLimiter() {
+ List<CompletableFuture<Void>> consumerCheckFutures = new ArrayList<>();
+ subscriptions.forEach((subName, sub) -> sub.getConsumers().forEach(consumer -> {
+ consumerCheckFutures.add(consumer.checkPermissionsAsync().thenRun(() -> {
+ Dispatcher dispatcher = sub.getDispatcher();
Review Comment:
I have update it. PTAL again @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
[GitHub] [pulsar] AnonHxy commented on pull request #15789: [improve][broker]tidy update subscriptions dispatcher rate-limiter
Posted by GitBox <gi...@apache.org>.
AnonHxy commented on PR #15789:
URL: https://github.com/apache/pulsar/pull/15789#issuecomment-1193933590
> @AnonHxy Please help create a PR for branch-2.10, I'm not able to cherry-pick it due to lots of conflicts with branch-2.10
The PR has been created, PTAL @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 #15789: [broker]tidy update subscriptions dispatcher rate-limiter
Posted by GitBox <gi...@apache.org>.
AnonHxy commented on PR #15789:
URL: https://github.com/apache/pulsar/pull/15789#issuecomment-1140217553
/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] codelipenghui commented on pull request #15789: [improve][broker]tidy update subscriptions dispatcher rate-limiter
Posted by GitBox <gi...@apache.org>.
codelipenghui commented on PR #15789:
URL: https://github.com/apache/pulsar/pull/15789#issuecomment-1193517724
@AnonHxy Please help create a PR for branch-2.10, I'm not able to cherry-pick it due to lots of conflicts with branch-2.10
--
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 #15789: [broker]tidy update subscriptions dispatcher rate-limiter
Posted by GitBox <gi...@apache.org>.
AnonHxy commented on code in PR #15789:
URL: https://github.com/apache/pulsar/pull/15789#discussion_r884084944
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentTopic.java:
##########
@@ -3066,6 +3036,19 @@ public void onUpdate(TopicPolicies policies) {
});
}
+ private CompletableFuture<Void> updateSubscriptionsDispatcherRateLimiter() {
+ List<CompletableFuture<Void>> consumerCheckFutures = new ArrayList<>();
+ subscriptions.forEach((subName, sub) -> sub.getConsumers().forEach(consumer -> {
+ consumerCheckFutures.add(consumer.checkPermissionsAsync().thenRun(() -> {
+ Dispatcher dispatcher = sub.getDispatcher();
+ if (dispatcher != null) {
+ dispatcher.updateRateLimiter();
+ }
+ }));
+ }));
+ return CompletableFuture.allOf(consumerCheckFutures.toArray(new CompletableFuture[0]));
Review Comment:
They are same. `FutureUtil.waitForAll(consumerCheckFutures)` make less modification for code. I will update 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.
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 merged pull request #15789: [improve][broker]tidy update subscriptions dispatcher rate-limiter
Posted by GitBox <gi...@apache.org>.
Jason918 merged PR #15789:
URL: https://github.com/apache/pulsar/pull/15789
--
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 #15789: [broker]tidy update subscriptions dispatcher rate-limiter
Posted by GitBox <gi...@apache.org>.
AnonHxy commented on PR #15789:
URL: https://github.com/apache/pulsar/pull/15789#issuecomment-1140377146
/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 #15789: [broker]tidy update subscriptions dispatcher rate-limiter
Posted by GitBox <gi...@apache.org>.
AnonHxy commented on PR #15789:
URL: https://github.com/apache/pulsar/pull/15789#issuecomment-1140451017
/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 #15789: [broker]tidy update subscriptions dispatcher rate-limiter
Posted by GitBox <gi...@apache.org>.
AnonHxy commented on PR #15789:
URL: https://github.com/apache/pulsar/pull/15789#issuecomment-1140273491
/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 #15789: [improve][broker]tidy update subscriptions dispatcher rate-limiter
Posted by GitBox <gi...@apache.org>.
AnonHxy commented on PR #15789:
URL: https://github.com/apache/pulsar/pull/15789#issuecomment-1144812889
/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 #15789: [improve][broker]tidy update subscriptions dispatcher rate-limiter
Posted by GitBox <gi...@apache.org>.
AnonHxy commented on PR #15789:
URL: https://github.com/apache/pulsar/pull/15789#issuecomment-1143980593
/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 #15789: [broker]tidy update subscriptions dispatcher rate-limiter
Posted by GitBox <gi...@apache.org>.
AnonHxy commented on PR #15789:
URL: https://github.com/apache/pulsar/pull/15789#issuecomment-1140235694
/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 a diff in pull request #15789: [broker]tidy update subscriptions dispatcher rate-limiter
Posted by GitBox <gi...@apache.org>.
Jason918 commented on code in PR #15789:
URL: https://github.com/apache/pulsar/pull/15789#discussion_r884060963
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentTopic.java:
##########
@@ -3066,6 +3036,19 @@ public void onUpdate(TopicPolicies policies) {
});
}
+ private CompletableFuture<Void> updateSubscriptionsDispatcherRateLimiter() {
+ List<CompletableFuture<Void>> consumerCheckFutures = new ArrayList<>();
+ subscriptions.forEach((subName, sub) -> sub.getConsumers().forEach(consumer -> {
+ consumerCheckFutures.add(consumer.checkPermissionsAsync().thenRun(() -> {
+ Dispatcher dispatcher = sub.getDispatcher();
+ if (dispatcher != null) {
+ dispatcher.updateRateLimiter();
+ }
+ }));
+ }));
+ return CompletableFuture.allOf(consumerCheckFutures.toArray(new CompletableFuture[0]));
Review Comment:
Is this different with `FutureUtil.waitForAll(consumerCheckFutures)`?
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentTopic.java:
##########
@@ -3066,6 +3036,19 @@ public void onUpdate(TopicPolicies policies) {
});
}
+ private CompletableFuture<Void> updateSubscriptionsDispatcherRateLimiter() {
+ List<CompletableFuture<Void>> consumerCheckFutures = new ArrayList<>();
+ subscriptions.forEach((subName, sub) -> sub.getConsumers().forEach(consumer -> {
+ consumerCheckFutures.add(consumer.checkPermissionsAsync().thenRun(() -> {
+ Dispatcher dispatcher = sub.getDispatcher();
Review Comment:
It seems we don't need check `dispatcher` for each consumer. Previous codes in L2416 make more sense.
--
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 #15789: [broker]tidy update subscriptions dispatcher rate-limiter
Posted by GitBox <gi...@apache.org>.
AnonHxy commented on PR #15789:
URL: https://github.com/apache/pulsar/pull/15789#issuecomment-1140357320
/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 #15789: [broker]tidy update subscriptions dispatcher rate-limiter
Posted by GitBox <gi...@apache.org>.
AnonHxy commented on PR #15789:
URL: https://github.com/apache/pulsar/pull/15789#issuecomment-1140618409
/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