You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pulsar.apache.org by "lhotari (via GitHub)" <gi...@apache.org> on 2023/02/02 07:31:20 UTC
[GitHub] [pulsar] lhotari opened a new pull request, #19388: [improve][broker] Reduce calls on metadata store / ZK event thread & Netty threads in PersistentTopic
lhotari opened a new pull request, #19388:
URL: https://github.com/apache/pulsar/pull/19388
### Motivation
It is not recommended to run possible blocking operations on Zookeeper event thread, Metadata Store notification thread or Netty IO threads.
Execution could happen unintentionally on these threads since `CompletableFuture` callbacks (`thenCompose`, `thenAccept`, `thenRun`, `whenComplete`, ...) run on the thread that completes the `CompletableFuture`.
In PersistentTopic, there are a few locations where there are potential issues. It's better to run the callbacks on another thread pool when there's a heavy operation or any operation that uses synchronization or locks.
### Modifications
- replace `thenAccept` -> `thenAcceptAsync` and `thenRun` -> `thenRunAsync` in a few call chains
- use `brokerService.getTopicOrderedExecutor().chooseThread(topic)` as the executor.
### Documentation
<!-- DO NOT REMOVE THIS SECTION. CHECK THE PROPER BOX ONLY. -->
- [ ] `doc` <!-- Your PR contains doc changes. -->
- [ ] `doc-required` <!-- Your PR changes impact docs and you will update later -->
- [x] `doc-not-needed` <!-- Your PR changes do not impact docs -->
- [ ] `doc-complete` <!-- Docs have been already added -->
--
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 merged pull request #19388: [improve][broker] Reduce calls on metadata store / ZK event thread & Netty threads in PersistentTopic
Posted by "lhotari (via GitHub)" <gi...@apache.org>.
lhotari merged PR #19388:
URL: https://github.com/apache/pulsar/pull/19388
--
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 #19388: [improve][broker] Reduce calls on metadata store / ZK event thread & Netty threads in PersistentTopic
Posted by "lhotari (via GitHub)" <gi...@apache.org>.
lhotari commented on code in PR #19388:
URL: https://github.com/apache/pulsar/pull/19388#discussion_r1094398520
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentTopic.java:
##########
@@ -359,7 +359,7 @@ public CompletableFuture<Void> initialize() {
this.isEncryptionRequired = policies.encryption_required;
isAllowAutoUpdateSchema = policies.is_allow_auto_update_schema;
- })
+ }, brokerService.getTopicOrderedExecutor().chooseThread(topic))
Review Comment:
thanks for the suggestion, I'll do that.
--
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 diff in pull request #19388: [improve][broker] Reduce calls on metadata store / ZK event thread & Netty threads in PersistentTopic
Posted by "eolivelli (via GitHub)" <gi...@apache.org>.
eolivelli commented on code in PR #19388:
URL: https://github.com/apache/pulsar/pull/19388#discussion_r1094384342
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentTopic.java:
##########
@@ -359,7 +359,7 @@ public CompletableFuture<Void> initialize() {
this.isEncryptionRequired = policies.encryption_required;
isAllowAutoUpdateSchema = policies.is_allow_auto_update_schema;
- })
+ }, brokerService.getTopicOrderedExecutor().chooseThread(topic))
Review Comment:
in the Dispatcher we cached the result of brokerService.getTopicOrderedExecutor().chooseThread(topic) into an instance field, this will also reduce the waste of resources due to choosing the thread (and make the code cleaner)
--
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] nicoloboschi commented on a diff in pull request #19388: [improve][broker] Reduce calls on metadata store / ZK event thread & Netty threads in PersistentTopic
Posted by "nicoloboschi (via GitHub)" <gi...@apache.org>.
nicoloboschi commented on code in PR #19388:
URL: https://github.com/apache/pulsar/pull/19388#discussion_r1094376358
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentTopic.java:
##########
@@ -359,7 +359,7 @@ public CompletableFuture<Void> initialize() {
this.isEncryptionRequired = policies.encryption_required;
isAllowAutoUpdateSchema = policies.is_allow_auto_update_schema;
- })
+ }, brokerService.getTopicOrderedExecutor().chooseThread(topic))
Review Comment:
it'd be good to create a private method to get an executor instead of repeating this call many times
--
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