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/07/14 12:02:14 UTC
[GitHub] [pulsar] eolivelli opened a new pull request, #16603: Shared subscription: run filters in a separate (per-subscription) thread (dispatcherDispatchMessagesInSubscriptionThread)
eolivelli opened a new pull request, #16603:
URL: https://github.com/apache/pulsar/pull/16603
### Motivation
Server side filters may be very heavy weight, and currently they are running (very often) in the same thread that handles writes and also all the subscriptions.
We want to improve performances in presence of heavy server side filters:
- subscriptions should not impact each other
- dispatching should not impact writes
### Modifications
Add a new flag, `dispatcherDispatchMessagesInSubscriptionThread`, enabled by default, to dispatch messages to consumers in a Shared subscription using a separate thread than the BK executor thread.
With this patch we choose a thread depending on the topic + subscription name and we run the actual dispatching of messages (sendMessagesToConsumers) in such thread.
### Verifying this change
I did some manual testing with a dummy filter that slows down (Thread.sleep(100)) consumption and even with a simply workload (50k msg/s) I have verified that:
- with this patch the noise over other subscription is smaller (without this change a second, non filtered subscription worked at 10% speed)
- the impact on writes is smaller
--
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 #16603: [enh] Broker - Shared subscription: run filters in a separate (per-subscription) thread (dispatcherDispatchMessagesInSubscriptionThread)
Posted by GitBox <gi...@apache.org>.
eolivelli commented on code in PR #16603:
URL: https://github.com/apache/pulsar/pull/16603#discussion_r921153339
##########
conf/broker.conf:
##########
@@ -403,6 +403,9 @@ dispatchThrottlingOnNonBacklogConsumerEnabled=true
# Max number of entries to read from bookkeeper. By default it is 100 entries.
dispatcherMaxReadBatchSize=100
+# Dispatch messages and execute broker side filters in a per-subscription thread
+dispatcherDispatchMessagesInSubscriptionThread=true
Review Comment:
I am adding this flag only to rollback in case of problem with particular workloads.
We are doing performance testing, I believe that this new behaviour is generally better, not only for the case of server side filters, but especially in these cases:
- multiple subscriptions on the same topic
- concurrent produce and consume
--
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 #16603: [enh] Broker - Shared subscription: run filters in a separate (per-subscription) thread (dispatcherDispatchMessagesInSubscriptionThread)
Posted by GitBox <gi...@apache.org>.
eolivelli commented on code in PR #16603:
URL: https://github.com/apache/pulsar/pull/16603#discussion_r926480879
##########
pulsar-broker/src/test/java/org/apache/pulsar/broker/service/persistent/PersistentStickyKeyDispatcherMultipleConsumersTest.java:
##########
@@ -107,6 +109,9 @@ public void setup() throws Exception {
HierarchyTopicPolicies topicPolicies = new HierarchyTopicPolicies();
topicPolicies.getMaxConsumersPerSubscription().updateBrokerValue(0);
+ OrderedExecutor orderedExecutor = OrderedExecutor.newBuilder().build();
Review Comment:
good 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] eolivelli commented on a diff in pull request #16603: [enh] Broker - Shared subscription: run filters in a separate (per-subscription) thread (dispatcherDispatchMessagesInSubscriptionThread)
Posted by GitBox <gi...@apache.org>.
eolivelli commented on code in PR #16603:
URL: https://github.com/apache/pulsar/pull/16603#discussion_r921980499
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentDispatcherMultipleConsumers.java:
##########
@@ -520,10 +521,19 @@ public synchronized void readEntriesComplete(List<Entry> entries, Object ctx) {
log.debug("[{}] Distributing {} messages to {} consumers", name, entries.size(), consumerList.size());
}
- sendMessagesToConsumers(readType, entries);
+ if (serviceConfig.isDispatcherDispatchMessagesInSubscriptionThread()) {
+ // dispatch messages to a separate thread, but still in order for this subscription
+ // sendMessagesToConsumers is responsible for running broker-side filters
+ // that may be quite expensive
+ topic.getBrokerService().getTopicOrderedExecutor()
+ .executeOrdered(name,
+ safeRun(() -> sendMessagesToConsumers(readType, entries)));
Review Comment:
@codelipenghui I have pinned the Subscription to a thread.
I think that this is what you suggested
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] eolivelli commented on a diff in pull request #16603: [enh] Broker - Shared subscription: run filters in a separate (per-subscription) thread (dispatcherDispatchMessagesInSubscriptionThread)
Posted by GitBox <gi...@apache.org>.
eolivelli commented on code in PR #16603:
URL: https://github.com/apache/pulsar/pull/16603#discussion_r921893502
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentDispatcherMultipleConsumers.java:
##########
@@ -520,10 +521,19 @@ public synchronized void readEntriesComplete(List<Entry> entries, Object ctx) {
log.debug("[{}] Distributing {} messages to {} consumers", name, entries.size(), consumerList.size());
}
- sendMessagesToConsumers(readType, entries);
+ if (serviceConfig.isDispatcherDispatchMessagesInSubscriptionThread()) {
+ // dispatch messages to a separate thread, but still in order for this subscription
+ // sendMessagesToConsumers is responsible for running broker-side filters
+ // that may be quite expensive
+ topic.getBrokerService().getTopicOrderedExecutor()
+ .executeOrdered(name,
Review Comment:
`do we care about potential (unchecked) RejectedExecutionException here?`
we usually don't deal with RejectedExecutionException in Dispatcher code.
in any case even if the execution is rejected the Dispatcher will read more entries for some other reasons
`we may need to prevent this code from submitting unlimited amount of runnables into that executor / handle long backlog.`
This shouldn't be a problem because there is a "natural" backpressure mechanism here, we trigger this code when new entries are read from storage (BK/Offloaders) but we request new entries after dispatching them to Consumers.
So usually there is only one of this tasks that is pending for the subscription.
--
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 merged pull request #16603: [enh] Broker - Shared subscription: run filters in a separate (per-subscription) thread (dispatcherDispatchMessagesInSubscriptionThread)
Posted by GitBox <gi...@apache.org>.
codelipenghui merged PR #16603:
URL: https://github.com/apache/pulsar/pull/16603
--
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] mattisonchao commented on pull request #16603: [enh] Broker - Shared subscription: run filters in a separate (per-subscription) thread (dispatcherDispatchMessagesInSubscriptionThread)
Posted by GitBox <gi...@apache.org>.
mattisonchao commented on PR #16603:
URL: https://github.com/apache/pulsar/pull/16603#issuecomment-1204604963
This enhancement may fix the `StackOverFlow` exception.
```
2022-08-01T15:30:08,243+0000 [pulsar-ph-amqp-41-1] WARN org.apache.bookkeeper.mledger.impl.EntryCacheImpl - failed to read entries for 467924-467924:46
java.lang.StackOverflowError: null
at jdk.internal.org.objectweb.asm.ClassWriter.newStringishItem(ClassWriter.java:1189) ~[?:?]
at jdk.internal.org.objectweb.asm.MethodWriter.visitTypeInsn(MethodWriter.java:866) ~[?:?]
at jdk.internal.org.objectweb.asm.MethodVisitor.visitTypeInsn(MethodVisitor.java:430) ~[?:?]
at java.lang.invoke.TypeConvertingMethodAdapter.cast(TypeConvertingMethodAdapter.java:188) ~[?:?]
at java.lang.invoke.TypeConvertingMethodAdapter.convertType(TypeConvertingMethodAdapter.java:241) ~[?:?]
at java.lang.invoke.InnerClassLambdaMetafactory$ForwardingMethodGenerator.convertArgumentTypes(InnerClassLambdaMetafactory.java:499) ~[?:?]
at java.lang.invoke.InnerClassLambdaMetafactory$ForwardingMethodGenerator.generate(InnerClassLambdaMetafactory.java:472) ~[?:?]
at java.lang.invoke.InnerClassLambdaMetafactory.spinInnerClass(InnerClassLambdaMetafactory.java:294) ~[?:?]
at java.lang.invoke.InnerClassLambdaMetafactory.buildCallSite(InnerClassLambdaMetafactory.java:195) ~[?:?]
at java.lang.invoke.LambdaMetafactory.metafactory(LambdaMetafactory.java:329) ~[?:?]
at java.lang.invoke.BootstrapMethodInvoker.invoke(BootstrapMethodInvoker.java:127) ~[?:?]
at java.lang.invoke.CallSite.makeSite(CallSite.java:307) ~[?:?]
at java.lang.invoke.MethodHandleNatives.linkCallSiteImpl(MethodHandleNatives.java:258) ~[?:?]
at java.lang.invoke.MethodHandleNatives.linkCallSite(MethodHandleNatives.java:248) ~[?:?]
at org.apache.bookkeeper.mledger.impl.ManagedCursorImpl$12.readEntryFailed(ManagedCursorImpl.java:1398) ~[pulsar-managed-ledger-2.10.1.1.jar:2.10.1.1]
at org.apache.bookkeeper.mledger.impl.EntryCacheImpl.asyncReadEntry(EntryCacheImpl.java:195) ~[pulsar-managed-ledger-2.10.1.1.jar:2.10.1.1]
at org.apache.bookkeeper.mledger.impl.ManagedLedgerImpl.asyncReadEntry(ManagedLedgerImpl.java:1982) ~[pulsar-managed-ledger-2.10.1.1.jar:2.10.1.1]
at org.apache.bookkeeper.mledger.impl.ManagedLedgerImpl.asyncReadEntry(ManagedLedgerImpl.java:1897) ~[pulsar-managed-ledger-2.10.1.1.jar:2.10.1.1]
at org.apache.bookkeeper.mledger.impl.ManagedCursorImpl.lambda$asyncReplayEntries$11(ManagedCursorImpl.java:1413) ~[pulsar-managed-ledger-2.10.1.1.jar:2.10.1.1]
at java.util.stream.ForEachOps$ForEachOp$OfRef.accept(ForEachOps.java:183) ~[?:?]
at java.util.stream.ReferencePipeline$2$1.accept(ReferencePipeline.java:177) ~[?:?]
at java.util.TreeMap$KeySpliterator.forEachRemaining(TreeMap.java:2739) ~[?:?]
at java.util.stream.AbstractPipeline.copyInto(AbstractPipeline.java:484) ~[?:?]
at java.util.stream.AbstractPipeline.wrapAndCopyInto(AbstractPipeline.java:474) ~[?:?]
at java.util.stream.ForEachOps$ForEachOp.evaluateSequential(ForEachOps.java:150) ~[?:?]
at java.util.stream.ForEachOps$ForEachOp$OfRef.evaluateSequential(ForEachOps.java:173) ~[?:?]
at java.util.stream.AbstractPipeline.evaluate(AbstractPipeline.java:234) ~[?:?]
at java.util.stream.ReferencePipeline.forEach(ReferencePipeline.java:497) ~[?:?]
at org.apache.bookkeeper.mledger.impl.ManagedCursorImpl.asyncReplayEntries(ManagedCursorImpl.java:1407) ~[pulsar-managed-ledger-2.10.1.1.jar:2.10.1.1]
at org.apache.pulsar.broker.service.persistent.PersistentDispatcherMultipleConsumers.asyncReplayEntriesInOrder(PersistentDispatcherMultipleConsumers.java:405) ~[pulsar-pulsar-broker-2.10.1.1.jar:2.10.1.1]
at org.apache.pulsar.broker.service.persistent.PersistentDispatcherMultipleConsumers.readMoreEntries(PersistentDispatcherMultipleConsumers.java:255) ~[pulsar-pulsar-broker-2.10.1.1.jar:2.10.1.1]
at org.apache.pulsar.broker.service.persistent.PersistentDispatcherMultipleConsumers.sendMessagesToConsumers(PersistentDispatcherMultipleConsumers.java:636) ~[pulsar-pulsar-broker-2.10.1.1.jar:2.10.1.1]
at org.apache.pulsar.broker.service.persistent.PersistentDispatcherMultipleConsumers.readEntriesComplete(PersistentDispatcherMultipleConsumers.java:513) ~[pulsar-pulsar-broker-2.10.1.1.jar:2.10.1.1]
at org.apache.bookkeeper.mledger.impl.ManagedCursorImpl$12.readEntryComplete(ManagedCursorImpl.java:1388) ~[pulsar-managed-ledger-2.10.1.1.jar:2.10.1.1]
at org.apache.bookkeeper.mledger.impl.EntryCacheImpl.asyncReadEntry0(EntryCacheImpl.java:209) ~[pulsar-managed-ledger-2.10.1.1.jar:2.10.1.1]
at org.apache.bookkeeper.mledger.impl.EntryCacheImpl.asyncReadEntry(EntryCacheImpl.java:188) ~[pulsar-managed-ledger-2.10.1.1.jar:2.10.1.1]
at org.apache.bookkeeper.mledger.impl.ManagedLedgerImpl.asyncReadEntry(ManagedLedgerImpl.java:1982) ~[pulsar-managed-ledger-2.10.1.1.jar:2.10.1.1]
at org.apache.bookkeeper.mledger.impl.ManagedLedgerImpl.asyncReadEntry(ManagedLedgerImpl.java:1897) ~[pulsar-managed-ledger-2.10.1.1.jar:2.10.1.1]
at org.apache.bookkeeper.mledger.impl.ManagedCursorImpl.lambda$asyncReplayEntries$11(ManagedCursorImpl.java:1413) ~[pulsar-managed-ledger-2.10.1.1.jar:2.10.1.1]
at java.util.stream.ForEachOps$ForEachOp$OfRef.accept(ForEachOps.java:183) ~[?:?]
at java.util.stream.ReferencePipeline$2$1.accept(ReferencePipeline.java:177) ~[?:?]
at java.util.TreeMap$KeySpliterator.forEachRemaining(TreeMap.java:2739) ~[?:?]
at java.util.stream.AbstractPipeline.copyInto(AbstractPipeline.java:484) ~[?:?]
at java.util.stream.AbstractPipeline.wrapAndCopyInto(AbstractPipeline.java:474) ~[?:?]
at java.util.stream.ForEachOps$ForEachOp.evaluateSequential(ForEachOps.java:150) ~[?:?]
at java.util.stream.ForEachOps$ForEachOp$OfRef.evaluateSequential(ForEachOps.java:173) ~[?:?]
at java.util.stream.AbstractPipeline.evaluate(AbstractPipeline.java:234) ~[?:?]
at java.util.stream.ReferencePipeline.forEach(ReferencePipeline.java:497) ~[?:?]
at org.apache.bookkeeper.mledger.impl.ManagedCursorImpl.asyncReplayEntries(ManagedCursorImpl.java:1407) ~[pulsar-managed-ledger-2.10.1.1.jar:2.10.1.1]
at org.apache.pulsar.broker.service.persistent.PersistentDispatcherMultipleConsumers.asyncReplayEntriesInOrder(PersistentDispatcherMultipleConsumers.java:405) ~[pulsar-pulsar-broker-2.10.1.1.jar:2.10.1.1]
at org.apache.pulsar.broker.service.persistent.PersistentDispatcherMultipleConsumers.readMoreEntries(PersistentDispatcherMultipleConsumers.java:255) ~[pulsar-pulsar-broker-2.10.1.1.jar:2.10.1.1]
at org.apache.pulsar.broker.service.persistent.PersistentDispatcherMultipleConsumers.sendMessagesToConsumers(PersistentDispatcherMultipleConsumers.java:636) ~[pulsar-pulsar-broker-2.10.1.1.jar:2.10.1.1]
at org.apache.pulsar.broker.service.persistent.PersistentDispatcherMultipleConsumers.readEntriesComplete(PersistentDispatcherMultipleConsumers.java:513) ~[pulsar-pulsar-broker-2.10.1.1.jar:2.10.1.1]
at org.apache.bookkeeper.mledger.impl.ManagedCursorImpl$12.readEntryComplete(ManagedCursorImpl.java:1388) ~[pulsar-managed-ledger-2.10.1.1.jar:2.10.1.1]
at org.apache.bookkeeper.mledger.impl.EntryCacheImpl.asyncReadEntry0(EntryCacheImpl.java:209) ~[pulsar-managed-ledger-2.10.1.1.jar:2.10.1.1]
at org.apache.bookkeeper.mledger.impl.EntryCacheImpl.asyncReadEntry(EntryCacheImpl.java:188) ~[pulsar-managed-ledger-2.10.1.1.jar:2.10.1.1]
at org.apache.bookkeeper.mledger.impl.ManagedLedgerImpl.asyncReadEntry(ManagedLedgerImpl.java:1982) ~[pulsar-managed-ledger-2.10.1.1.jar:2.10.1.1]
at org.apache.bookkeeper.mledger.impl.ManagedLedgerImpl.asyncReadEntry(ManagedLedgerImpl.java:1897) ~[pulsar-managed-ledger-2.10.1.1.jar:2.10.1.1]
at org.apache.bookkeeper.mledger.impl.ManagedCursorImpl.lambda$asyncReplayEntries$11(ManagedCursorImpl.java:1413) ~[pulsar-managed-ledger-2.10.1.1.jar:2.10.1.1]
at java.util.stream.ForEachOps$ForEachOp$OfRef.accept(ForEachOps.java:183) ~[?:?]
at java.util.stream.ReferencePipeline$2$1.accept(ReferencePipeline.java:177) ~[?:?]
at java.util.TreeMap$KeySpliterator.forEachRemaining(TreeMap.java:2739) ~[?:?]
at java.util.stream.AbstractPipeline.copyInto(AbstractPipeline.java:484) ~[?:?]
at java.util.stream.AbstractPipeline.wrapAndCopyInto(AbstractPipeline.java:474) ~[?:?]
at java.util.stream.ForEachOps$ForEachOp.evaluateSequential(ForEachOps.java:150) ~[?:?]
at java.util.stream.ForEachOps$ForEachOp$OfRef.evaluateSequential(ForEachOps.java:173) ~[?:?]
at java.util.stream.AbstractPipeline.evaluate(AbstractPipeline.java:234) ~[?:?]
at java.util.stream.ReferencePipeline.forEach(ReferencePipeline.java:497) ~[?:?]
at org.apache.bookkeeper.mledger.impl.ManagedCursorImpl.asyncReplayEntries(ManagedCursorImpl.java:1407) ~[pulsar-managed-ledger-2.10.1.1.jar:2.10.1.1]
at org.apache.pulsar.broker.service.persistent.PersistentDispatcherMultipleConsumers.asyncReplayEntriesInOrder(PersistentDispatcherMultipleConsumers.java:405) ~[pulsar-pulsar-broker-2.10.1.1.jar:2.10.1.1]
at org.apache.pulsar.broker.service.persistent.PersistentDispatcherMultipleConsumers.readMoreEntries(PersistentDispatcherMultipleConsumers.java:255) ~[pulsar-pulsar-broker-2.10.1.1.jar:2.10.1.1]
at org.apache.pulsar.broker.service.persistent.PersistentDispatcherMultipleConsumers.sendMessagesToConsumers(PersistentDispatcherMultipleConsumers.java:636) ~[pulsar-pulsar-broker-2.10.1.1.jar:2.10.1.1]
at org.apache.pulsar.broker.service.persistent.PersistentDispatcherMultipleConsumers.readEntriesComplete(PersistentDispatcherMultipleConsumers.java:513) ~[pulsar-pulsar-broker-2.10.1.1.jar:2.10.1.1]
at org.apache.bookkeeper.mledger.impl.ManagedCursorImpl$12.readEntryComplete(ManagedCursorImpl.java:1388) ~[pulsar-managed-ledger-2.10.1.1.jar:2.10.1.1]
at org.apache.bookkeeper.mledger.impl.EntryCacheImpl.asyncReadEntry0(EntryCacheImpl.java:209) ~[pulsar-managed-ledger-2.10.1.1.jar:2.10.1.1]
at org.apache.bookkeeper.mledger.impl.EntryCacheImpl.asyncReadEntry(EntryCacheImpl.java:188) ~[pulsar-managed-ledger-2.10.1.1.jar:2.10.1.1]
at org.apache.bookkeeper.mledger.impl.ManagedLedgerImpl.asyncReadEntry(ManagedLedgerImpl.java:1982) ~[pulsar-managed-ledger-2.10.1.1.jar:2.10.1.1]
at org.apache.bookkeeper.mledger.impl.ManagedLedgerImpl.asyncReadEntry(ManagedLedgerImpl.java:1897) ~[pulsar-managed-ledger-2.10.1.1.jar:2.10.1.1]
at org.apache.bookkeeper.mledger.impl.ManagedCursorImpl.lambda$asyncReplayEntries$11(ManagedCursorImpl.java:1413) ~[pulsar-managed-ledger-2.10.1.1.jar:2.10.1.1]
at java.util.stream.ForEachOps$ForEachOp$OfRef.accept(ForEachOps.java:183) ~[?:?]
at java.util.stream.ReferencePipeline$2$1.accept(ReferencePipeline.java:177) ~[?:?]
at java.util.TreeMap$KeySpliterator.forEachRemaining(TreeMap.java:2739) ~[?:?]
at java.util.stream.AbstractPipeline.copyInto(AbstractPipeline.java:484) ~[?:?]
at java.util.stream.AbstractPipeline.wrapAndCopyInto(AbstractPipeline.java:474) ~[?:?]
at java.util.stream.ForEachOps$ForEachOp.evaluateSequential(ForEachOps.java:150) ~[?:?]
at java.util.stream.ForEachOps$ForEachOp$OfRef.evaluateSequential(ForEachOps.java:173) ~[?:?]
at java.util.stream.AbstractPipeline.evaluate(AbstractPipeline.java:234) ~[?:?]
at java.util.stream.ReferencePipeline.forEach(ReferencePipeline.java:497) ~[?:?]
at org.apache.bookkeeper.mledger.impl.ManagedCursorImpl.asyncReplayEntries(ManagedCursorImpl.java:1407) ~[pulsar-managed-ledger-2.10.1.1.jar:2.10.1.1]
at org.apache.pulsar.broker.service.persistent.PersistentDispatcherMultipleConsumers.asyncReplayEntriesInOrder(PersistentDispatcherMultipleConsumers.java:405) ~[pulsar-pulsar-broker-2.10.1.1.jar:2.10.1.1]
```
--
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] dlg99 commented on a diff in pull request #16603: [enh] Broker - Shared subscription: run filters in a separate (per-subscription) thread (dispatcherDispatchMessagesInSubscriptionThread)
Posted by GitBox <gi...@apache.org>.
dlg99 commented on code in PR #16603:
URL: https://github.com/apache/pulsar/pull/16603#discussion_r921383719
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentDispatcherMultipleConsumers.java:
##########
@@ -520,10 +521,19 @@ public synchronized void readEntriesComplete(List<Entry> entries, Object ctx) {
log.debug("[{}] Distributing {} messages to {} consumers", name, entries.size(), consumerList.size());
}
- sendMessagesToConsumers(readType, entries);
+ if (serviceConfig.isDispatcherDispatchMessagesInSubscriptionThread()) {
+ // dispatch messages to a separate thread, but still in order for this subscription
+ // sendMessagesToConsumers is responsible for running broker-side filters
+ // that may be quite expensive
+ topic.getBrokerService().getTopicOrderedExecutor()
+ .executeOrdered(name,
Review Comment:
1. do we care about potential (unchecked) RejectedExecutionException here?
2. we may need to prevent this code from submitting unlimited amount of runnables into that executor / handle long backlog.
--
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 #16603: [enh] Broker - Shared subscription: run filters in a separate (per-subscription) thread (dispatcherDispatchMessagesInSubscriptionThread)
Posted by GitBox <gi...@apache.org>.
eolivelli commented on code in PR #16603:
URL: https://github.com/apache/pulsar/pull/16603#discussion_r921154437
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentDispatcherMultipleConsumers.java:
##########
@@ -520,10 +521,19 @@ public synchronized void readEntriesComplete(List<Entry> entries, Object ctx) {
log.debug("[{}] Distributing {} messages to {} consumers", name, entries.size(), consumerList.size());
}
- sendMessagesToConsumers(readType, entries);
+ if (serviceConfig.isDispatcherDispatchMessagesInSubscriptionThread()) {
+ // dispatch messages to a separate thread, but still in order for this subscription
+ // sendMessagesToConsumers is responsible for running broker-side filters
+ // that may be quite expensive
+ topic.getBrokerService().getTopicOrderedExecutor()
+ .executeOrdered(name,
+ safeRun(() -> sendMessagesToConsumers(readType, entries)));
Review Comment:
this is an interesting point.
Let me investigate.
I am not sure about the threadpool that we can use here as "pinned", I would like to pick a thread from "TopicOrderedExecutor"
--
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 #16603: [enh] Broker - Shared subscription: run filters in a separate (per-subscription) thread (dispatcherDispatchMessagesInSubscriptionThread)
Posted by GitBox <gi...@apache.org>.
eolivelli commented on code in PR #16603:
URL: https://github.com/apache/pulsar/pull/16603#discussion_r921890930
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentDispatcherMultipleConsumers.java:
##########
@@ -520,10 +521,19 @@ public synchronized void readEntriesComplete(List<Entry> entries, Object ctx) {
log.debug("[{}] Distributing {} messages to {} consumers", name, entries.size(), consumerList.size());
}
- sendMessagesToConsumers(readType, entries);
+ if (serviceConfig.isDispatcherDispatchMessagesInSubscriptionThread()) {
+ // dispatch messages to a separate thread, but still in order for this subscription
+ // sendMessagesToConsumers is responsible for running broker-side filters
+ // that may be quite expensive
+ topic.getBrokerService().getTopicOrderedExecutor()
+ .executeOrdered(name,
+ safeRun(() -> sendMessagesToConsumers(readType, entries)));
+ } else {
+ sendMessagesToConsumers(readType, entries);
+ }
}
- protected void sendMessagesToConsumers(ReadType readType, List<Entry> entries) {
+ protected synchronized void sendMessagesToConsumers(ReadType readType, List<Entry> entries) {
Review Comment:
there is no difference.
I think that this is not a problem.
@codelipenghui do you agree ?
--
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] mattisonchao commented on pull request #16603: [enh] Broker - Shared subscription: run filters in a separate (per-subscription) thread (dispatcherDispatchMessagesInSubscriptionThread)
Posted by GitBox <gi...@apache.org>.
mattisonchao commented on PR #16603:
URL: https://github.com/apache/pulsar/pull/16603#issuecomment-1207559466
Hi, @gaoran10
I don't think cherry-picking the feature with a new configuration to another old branch is a good idea.
--
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 #16603: [enh] Broker - Shared subscription: run filters in a separate (per-subscription) thread (dispatcherDispatchMessagesInSubscriptionThread)
Posted by GitBox <gi...@apache.org>.
eolivelli commented on PR #16603:
URL: https://github.com/apache/pulsar/pull/16603#issuecomment-1192342006
/pulsarbot rerun-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] github-actions[bot] commented on pull request #16603: [enh] Broker - Shared subscription: run filters in a separate (per-subscription) thread (dispatcherDispatchMessagesInSubscriptionThread)
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #16603:
URL: https://github.com/apache/pulsar/pull/16603#issuecomment-1185272033
@eolivelli Please provide a correct documentation label for your PR.
Instructions see [Pulsar Documentation Label Guide](https://docs.google.com/document/d/1Qw7LHQdXWBW9t2-r-A7QdFDBwmZh6ytB4guwMoXHqc0).
--
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] momo-jun commented on pull request #16603: [enh] Broker - Shared subscription: run filters in a separate (per-subscription) thread (dispatcherDispatchMessagesInSubscriptionThread)
Posted by GitBox <gi...@apache.org>.
momo-jun commented on PR #16603:
URL: https://github.com/apache/pulsar/pull/16603#issuecomment-1216195998
@eolivelli just wanted to check in - any docs required to be updated for this improvement?
--
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] github-actions[bot] commented on pull request #16603: Shared subscription: run filters in a separate (per-subscription) thread (dispatcherDispatchMessagesInSubscriptionThread)
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #16603:
URL: https://github.com/apache/pulsar/pull/16603#issuecomment-1184371524
@eolivelli Please provide a correct documentation label for your PR.
Instructions see [Pulsar Documentation Label Guide](https://docs.google.com/document/d/1Qw7LHQdXWBW9t2-r-A7QdFDBwmZh6ytB4guwMoXHqc0).
--
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 #16603: [enh] Broker - Shared subscription: run filters in a separate (per-subscription) thread (dispatcherDispatchMessagesInSubscriptionThread)
Posted by GitBox <gi...@apache.org>.
eolivelli commented on code in PR #16603:
URL: https://github.com/apache/pulsar/pull/16603#discussion_r921151871
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentDispatcherMultipleConsumers.java:
##########
@@ -520,10 +521,19 @@ public synchronized void readEntriesComplete(List<Entry> entries, Object ctx) {
log.debug("[{}] Distributing {} messages to {} consumers", name, entries.size(), consumerList.size());
}
- sendMessagesToConsumers(readType, entries);
+ if (serviceConfig.isDispatcherDispatchMessagesInSubscriptionThread()) {
+ // dispatch messages to a separate thread, but still in order for this subscription
+ // sendMessagesToConsumers is responsible for running broker-side filters
+ // that may be quite expensive
+ topic.getBrokerService().getTopicOrderedExecutor()
+ .executeOrdered(name,
+ safeRun(() -> sendMessagesToConsumers(readType, entries)));
+ } else {
+ sendMessagesToConsumers(readType, entries);
+ }
}
- protected void sendMessagesToConsumers(ReadType readType, List<Entry> entries) {
+ protected synchronized void sendMessagesToConsumers(ReadType readType, List<Entry> entries) {
Review Comment:
sendMessagesToConsumers is usually called by readEntriesComplete, that is already `synchronized`
https://github.com/apache/pulsar/blob/019493dbcf8e86c9caff71a88adc03aed5b1fbc8/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentDispatcherMultipleConsumers.java#L492
the JVM is smart enough to understand that there is no need for extra work for `synchronized`
--
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] mattisonchao commented on pull request #16603: [enh] Broker - Shared subscription: run filters in a separate (per-subscription) thread (dispatcherDispatchMessagesInSubscriptionThread)
Posted by GitBox <gi...@apache.org>.
mattisonchao commented on PR #16603:
URL: https://github.com/apache/pulsar/pull/16603#issuecomment-1204607298
And this PR has bug, the fix PR at here: https://github.com/apache/pulsar/pull/16812
--
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 #16603: [enh] Broker - Shared subscription: run filters in a separate (per-subscription) thread (dispatcherDispatchMessagesInSubscriptionThread)
Posted by GitBox <gi...@apache.org>.
lhotari commented on code in PR #16603:
URL: https://github.com/apache/pulsar/pull/16603#discussion_r926461398
##########
pulsar-broker/src/test/java/org/apache/pulsar/broker/service/persistent/PersistentStickyKeyDispatcherMultipleConsumersTest.java:
##########
@@ -107,6 +109,9 @@ public void setup() throws Exception {
HierarchyTopicPolicies topicPolicies = new HierarchyTopicPolicies();
topicPolicies.getMaxConsumersPerSubscription().updateBrokerValue(0);
+ OrderedExecutor orderedExecutor = OrderedExecutor.newBuilder().build();
Review Comment:
shouldn't this use the field?
```suggestion
orderedExecutor = OrderedExecutor.newBuilder().build();
```
--
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] michaeljmarshall commented on a diff in pull request #16603: [enh] Broker - Shared subscription: run filters in a separate (per-subscription) thread (dispatcherDispatchMessagesInSubscriptionThread)
Posted by GitBox <gi...@apache.org>.
michaeljmarshall commented on code in PR #16603:
URL: https://github.com/apache/pulsar/pull/16603#discussion_r921420293
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentDispatcherMultipleConsumers.java:
##########
@@ -520,10 +521,19 @@ public synchronized void readEntriesComplete(List<Entry> entries, Object ctx) {
log.debug("[{}] Distributing {} messages to {} consumers", name, entries.size(), consumerList.size());
}
- sendMessagesToConsumers(readType, entries);
+ if (serviceConfig.isDispatcherDispatchMessagesInSubscriptionThread()) {
+ // dispatch messages to a separate thread, but still in order for this subscription
+ // sendMessagesToConsumers is responsible for running broker-side filters
+ // that may be quite expensive
+ topic.getBrokerService().getTopicOrderedExecutor()
+ .executeOrdered(name,
+ safeRun(() -> sendMessagesToConsumers(readType, entries)));
+ } else {
+ sendMessagesToConsumers(readType, entries);
+ }
}
- protected void sendMessagesToConsumers(ReadType readType, List<Entry> entries) {
+ protected synchronized void sendMessagesToConsumers(ReadType readType, List<Entry> entries) {
Review Comment:
Alternatively, I think you could add a `synchronized (this)` block around the runnable's call to this method.
--
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 #16603: [enh] Broker - Shared subscription: run filters in a separate (per-subscription) thread (dispatcherDispatchMessagesInSubscriptionThread)
Posted by GitBox <gi...@apache.org>.
eolivelli commented on code in PR #16603:
URL: https://github.com/apache/pulsar/pull/16603#discussion_r921893502
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentDispatcherMultipleConsumers.java:
##########
@@ -520,10 +521,19 @@ public synchronized void readEntriesComplete(List<Entry> entries, Object ctx) {
log.debug("[{}] Distributing {} messages to {} consumers", name, entries.size(), consumerList.size());
}
- sendMessagesToConsumers(readType, entries);
+ if (serviceConfig.isDispatcherDispatchMessagesInSubscriptionThread()) {
+ // dispatch messages to a separate thread, but still in order for this subscription
+ // sendMessagesToConsumers is responsible for running broker-side filters
+ // that may be quite expensive
+ topic.getBrokerService().getTopicOrderedExecutor()
+ .executeOrdered(name,
Review Comment:
`do we care about potential (unchecked) RejectedExecutionException here?`
we usually don't deal with RejectedExecutionException in Dispatcher code.
in any case even if the execution is rejected the Dispatcher will read more entries for some other reasons
`we may need to prevent this code from submitting unlimited amount of runnables into that executor / handle long backlog.`
This shouldn't be a problem because there is a "natural" backpressure mechanism here, we trigger this code when new entries are read from storage (BK/Offloaders) but we request new reads from Storage after dispatching them to Consumers.
So usually there is only one of this tasks that is pending for the subscription.
--
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 a diff in pull request #16603: [enh] Broker - Shared subscription: run filters in a separate (per-subscription) thread (dispatcherDispatchMessagesInSubscriptionThread)
Posted by GitBox <gi...@apache.org>.
codelipenghui commented on code in PR #16603:
URL: https://github.com/apache/pulsar/pull/16603#discussion_r921110853
##########
conf/broker.conf:
##########
@@ -403,6 +403,9 @@ dispatchThrottlingOnNonBacklogConsumerEnabled=true
# Max number of entries to read from bookkeeper. By default it is 100 entries.
dispatcherMaxReadBatchSize=100
+# Dispatch messages and execute broker side filters in a per-subscription thread
+dispatcherDispatchMessagesInSubscriptionThread=true
Review Comment:
Should we keep it false by default? Most of the users don't have broker-side filters. Enabled by default will introduce more thread switching.
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentDispatcherMultipleConsumers.java:
##########
@@ -520,10 +521,19 @@ public synchronized void readEntriesComplete(List<Entry> entries, Object ctx) {
log.debug("[{}] Distributing {} messages to {} consumers", name, entries.size(), consumerList.size());
}
- sendMessagesToConsumers(readType, entries);
+ if (serviceConfig.isDispatcherDispatchMessagesInSubscriptionThread()) {
+ // dispatch messages to a separate thread, but still in order for this subscription
+ // sendMessagesToConsumers is responsible for running broker-side filters
+ // that may be quite expensive
+ topic.getBrokerService().getTopicOrderedExecutor()
+ .executeOrdered(name,
+ safeRun(() -> sendMessagesToConsumers(readType, entries)));
Review Comment:
We can have a pinned executor instead of selecting the executor for each operation.
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentDispatcherMultipleConsumers.java:
##########
@@ -520,10 +521,19 @@ public synchronized void readEntriesComplete(List<Entry> entries, Object ctx) {
log.debug("[{}] Distributing {} messages to {} consumers", name, entries.size(), consumerList.size());
}
- sendMessagesToConsumers(readType, entries);
+ if (serviceConfig.isDispatcherDispatchMessagesInSubscriptionThread()) {
+ // dispatch messages to a separate thread, but still in order for this subscription
+ // sendMessagesToConsumers is responsible for running broker-side filters
+ // that may be quite expensive
+ topic.getBrokerService().getTopicOrderedExecutor()
+ .executeOrdered(name,
+ safeRun(() -> sendMessagesToConsumers(readType, entries)));
+ } else {
+ sendMessagesToConsumers(readType, entries);
+ }
}
- protected void sendMessagesToConsumers(ReadType readType, List<Entry> entries) {
+ protected synchronized void sendMessagesToConsumers(ReadType readType, List<Entry> entries) {
Review Comment:
It's better to have a check and only apply the lock if `dispatcherDispatchMessagesInSubscriptionThread` is enabled.
--
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] github-actions[bot] commented on pull request #16603: [enh] Broker - Shared subscription: run filters in a separate (per-subscription) thread (dispatcherDispatchMessagesInSubscriptionThread)
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #16603:
URL: https://github.com/apache/pulsar/pull/16603#issuecomment-1207560062
@eolivelli Please provide a correct documentation label for your PR.
Instructions see [Pulsar Documentation Label Guide](https://docs.google.com/document/d/1Qw7LHQdXWBW9t2-r-A7QdFDBwmZh6ytB4guwMoXHqc0).
--
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