[Broker] Use shared executors for broker and geo-replication clients #13839

lhotari · 2022-01-19T13:29:58Z

Motivation

currently each broker client and geo-replication client will create a single-threaded internal/IO executor and an external/listener executer for each PulsarClientImpl instance. In addition there will be a separate Timer thread.
Instead of adding individual thread pools, it would be more efficient to share executors for all broker and geo-replication clients.

Modifications

create shared executors and timer at broker startup
reuse executors and timer for the broker client and the geo-replication clients.

…clients

merlimat · 2022-01-19T14:43:27Z

pulsar-broker-common/src/main/java/org/apache/pulsar/broker/ServiceConfiguration.java

+            doc = "Number of shared threads to use for broker clients."
+                    + " Default is set to `2 * Runtime.getRuntime().availableProcessors()`"
+    )
+    private int brokerClientNumIOThreads = 2 * Runtime.getRuntime().availableProcessors();


I'd say that we should prob reuse the broker IO threads here

The problem is that broker IO threads is a EventLoopGroup instance which is already used in the Pulsar Client.

pulsar/pulsar-broker/src/main/java/org/apache/pulsar/broker/PulsarService.java

Lines 319 to 320 in 9032a9a

this.ioEventLoopGroup = EventLoopUtil.newEventLoopGroup(config.getNumIOThreads(), config.isEnableBusyWait(),

new DefaultThreadFactory("pulsar-io"));

The Pulsar Client has this internal/IO and external/listener executors in addition to the eventLoopGroup.

pulsar/pulsar-client/src/main/java/org/apache/pulsar/client/impl/PulsarClientImpl.java

Lines 182 to 185 in 4591a21

this.externalExecutorProvider = externalExecutorProvider != null ? externalExecutorProvider :

new ExecutorProvider(conf.getNumListenerThreads(), "pulsar-external-listener");

this.internalExecutorProvider = internalExecutorProvider != null ? internalExecutorProvider :

new ExecutorProvider(conf.getNumIoThreads(), "pulsar-client-internal");

It might be a risky change in Pulsar Client to replace the internal executor to delegate directly to eventLoopGroup.

This is the reason why I think it's necessary to have a separate setting to control the amount of threads used to run the Pulsar Client internal executors. Before this PR each client has had a single threaded internal executor for each client instance.

Yes, I think the confusion comes from the name brokerClientNumIOThreads, since it's client internal executor and not the client event loop which is already shared.

Additionally, the client internal executor shouldn't really be used a lot in broker, since it's used mainly for dispatching the partitioned/multi topic consumers, so we could just default to 1 thread instead.

Yes, I think the confusion comes from the name brokerClientNumIOThreads, since it's client internal executor and not the client event loop which is already shared.

That's true. The Pulsar Client uses the numIOThreads parameter for configuring the number of threads for the eventLoopGroup and also for the internal/IO executor.

pulsar/pulsar-client/src/main/java/org/apache/pulsar/client/impl/PulsarClientImpl.java

Line 979 in 4591a21

return EventLoopUtil.newEventLoopGroup(conf.getNumIoThreads(), conf.isEnableBusyWait(), threadFactory);

pulsar/pulsar-client/src/main/java/org/apache/pulsar/client/impl/PulsarClientImpl.java

Lines 184 to 185 in 4591a21

this.internalExecutorProvider = internalExecutorProvider != null ? internalExecutorProvider :

new ExecutorProvider(conf.getNumIoThreads(), "pulsar-client-internal");

This is confusing. The internal executor was introduced in #8208 and that change uses numIOThreads parameter also for configuring the number of threads for the internal executor. @merlimat I wonder what would be a way to improve this situation?

Additionally, the client internal executor shouldn't really be used a lot in broker, since it's used mainly for dispatching the partitioned/multi topic consumers, so we could just default to 1 thread instead.

yes, that is true. I wonder if the value needs to be configurable at all? I'll remove brokerClientNumIOThreads key and just use the value 1 as I did for the external executor.

I made the changes now. @merlimat PTAL

- don't ever make it a daemon thread

michaeljmarshall · 2022-01-20T20:35:11Z

pulsar-broker/src/main/java/org/apache/pulsar/broker/PulsarService.java

+        // the external executor is not used in the broker client or replication clients since this executor is
+        // used for consumer listeners.
+        // since an instance is required, a single threaded shared instance is used for all broker client instances


Doesn't this hint at a problem in the java client? It looks like a listener is declared on consumer initialization. We could change the behavior so that it only gets a thread when there is a listener.

We'll still have the issue of creating an ExecutorProvider per client, so this change still makes sense, but it'd be cheaper for end users to avoid the thread creation for consumers that lack listeners.

I am going to open a PR to update the Java Consumer.

Doesn't this hint at a problem in the java client? It looks like a listener is declared on consumer initialization. We could change the behavior so that it only gets a thread when there is a listener.

Yes the client isn't optimal, but I doubt that it causes an actual measurable overhead. I think that the thread is just pinned out of the single shared thread and nothing will get run on it (in the current usage patterns when client is used in the broker).

Yes the client isn't optimal, but I doubt that it causes an actual measurable overhead. I think that the thread is just pinned out of the single shared thread and nothing will get run on it (in the current usage patterns when client is used in the broker).

I was thinking about user applications that have many consumers. I just looked a bit closer, and the upper limit here is NumListenerThreads in the client configuration. That is the max number of threads a client will spawn for consumer listeners. Was this a problem in the broker because we have many clients?

…13839) * [Broker] Use shared executors for broker clients and geo-replication clients * Remove brokerClientNumIOThreads configuration key and default to 1 * Revisit the shared timer creation - don't ever make it a daemon thread (cherry picked from commit 4924e6d)

[Broker] Use shared executors for broker clients and geo-replication …

4ef3525

…clients

lhotari added type/performance area/broker doc-not-needed Your PR changes do not impact docs labels Jan 19, 2022

lhotari added this to the 2.10.0 milestone Jan 19, 2022

lhotari requested review from ivankelly, merlimat, eolivelli, codelipenghui, congbobo184 and michaeljmarshall January 19, 2022 13:29

lhotari self-assigned this Jan 19, 2022

lhotari mentioned this pull request Jan 19, 2022

[Proxy] Use shared internal and external executor for broker clients #13834

Closed

merlimat reviewed Jan 19, 2022

View reviewed changes

lhotari added 2 commits January 20, 2022 06:36

Remove brokerClientNumIOThreads configuration key and default to 1

8f9f2ed

Revisit the shared timer creation

f1c8fec

- don't ever make it a daemon thread

lhotari requested a review from merlimat January 20, 2022 04:46

codelipenghui approved these changes Jan 20, 2022

View reviewed changes

merlimat approved these changes Jan 20, 2022

View reviewed changes

merlimat merged commit 4924e6d into apache:master Jan 20, 2022

michaeljmarshall reviewed Jan 20, 2022

View reviewed changes

congbobo184 added release/2.9.4 cherry-picked/branch-2.9 Archived: 2.9 is end of life labels Nov 10, 2022

codelipenghui mentioned this pull request Dec 27, 2022

PIP-234: Support using shared thread pool across multiple Pulsar client instance #19074

Open

sijie mentioned this pull request Dec 27, 2022

ISSUE-19074: PIP-234: Support using shared thread pool across multiple Pulsar client instance streamnative/pulsar-archived#5318

Open

lhotari mentioned this pull request Oct 10, 2024

[fix][client] Use dedicated executor for requests in BinaryProtoLookupService #23378

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Broker] Use shared executors for broker and geo-replication clients #13839

[Broker] Use shared executors for broker and geo-replication clients #13839

lhotari commented Jan 19, 2022 •

edited

Loading

merlimat Jan 19, 2022

lhotari Jan 19, 2022

merlimat Jan 19, 2022

lhotari Jan 20, 2022

lhotari Jan 20, 2022

lhotari Jan 20, 2022

merlimat Jan 20, 2022

michaeljmarshall Jan 20, 2022

lhotari Jan 20, 2022

michaeljmarshall Jan 20, 2022

	this.ioEventLoopGroup = EventLoopUtil.newEventLoopGroup(config.getNumIOThreads(), config.isEnableBusyWait(),
	new DefaultThreadFactory("pulsar-io"));

	this.externalExecutorProvider = externalExecutorProvider != null ? externalExecutorProvider :
	new ExecutorProvider(conf.getNumListenerThreads(), "pulsar-external-listener");
	this.internalExecutorProvider = internalExecutorProvider != null ? internalExecutorProvider :
	new ExecutorProvider(conf.getNumIoThreads(), "pulsar-client-internal");

[Broker] Use shared executors for broker and geo-replication clients #13839

[Broker] Use shared executors for broker and geo-replication clients #13839

Conversation

lhotari commented Jan 19, 2022 • edited Loading

Motivation

Modifications

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lhotari commented Jan 19, 2022 •

edited

Loading