Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Client] Support passing existing executor providers to the client #12037

Conversation

lhotari
Copy link
Member

@lhotari lhotari commented Sep 14, 2021

Motivation

In load and performance testing, there's a need to simulate production use cases and production workloads.
For this purpose, it would be useful to be able to share the thread pools used by Pulsar client instances in order to be able to run a large amount of Pulsar clients in a single JVM without the overhead of a lot of threads.
In the current solution, it's already possible to share the EventLoopGroup and HashedWheelTimer instances.
The solution for sharing the thread pools for the external / internal executors was missing. This PR adds support for that.

Example usage:

// shared thread pool related resources
ExecutorProvider internalExecutorProvider = new ExecutorProvider(8, "shared-internal-executor");
ExecutorProvider externalExecutorProvider = new ExecutorProvider(8, "shared-external-executor");
Timer sharedTimer = new HashedWheelTimer(getThreadFactory("shared-pulsar-timer"), 1, TimeUnit.MILLISECONDS);
EventLoopGroup sharedEventLoopGroup = new EpollEventLoopGroup();

// example of creating a client which uses the shared thread pools
PulsarClientImpl client = PulsarClientImpl.builder().conf(conf)
                .internalExecutorProvider(internalExecutorProvider)
                .externalExecutorProvider(externalExecutorProvider)
                .timer(sharedTimer)
                .eventLoopGroup(sharedEventLoopGroup)
                .build();

It seems that this would also improve the performance of the Pulsar Proxy since new thread pools for every client connection.

That happens in the Pulsar Proxy currently:

private PulsarClientImpl createClient(final ClientConfigurationData clientConf, final AuthData clientAuthData,
final String clientAuthMethod, final int protocolVersion) throws PulsarClientException {
this.connectionPool = new ProxyConnectionPool(clientConf, service.getWorkerGroup(),
() -> new ProxyClientCnx(clientConf, service.getWorkerGroup(), clientAuthRole, clientAuthData,
clientAuthMethod, protocolVersion));
return new PulsarClientImpl(clientConf, service.getWorkerGroup(), connectionPool, service.getTimer());
}

An optimization was added in #9802 for sharing the timer, but it would be useful to also share the internal / external executors.

Modifications

Refactor the code to add an internal builder class so that more constructors don't have to be added for advanced initialization of the PulsarClientImpl class.

@lhotari lhotari added this to the 2.9.0 milestone Sep 14, 2021
@lhotari lhotari self-assigned this Sep 14, 2021
@lhotari lhotari requested a review from eolivelli September 14, 2021 13:45
Copy link
Contributor

@eolivelli eolivelli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While I appreciate the feature and I see the benefits,
it looks like we are adding a new "official" way to build a Pulsar Client, that is not via the public API.

Do you want to use this only inside internal Pulsar tools ? the Proxy, Pulsar perf, Pulsar CLI tools ?

Why can't we add this support using the regular PulsarClient Builder API ?

@lhotari
Copy link
Member Author

lhotari commented Sep 14, 2021

Do you want to use this only inside internal Pulsar tools ? the Proxy, Pulsar perf, Pulsar CLI tools ?

yes, mainly an internal, unsupported API. Pulsar Proxy and Pulsar perf are the main internal use cases.

Why can't we add this support using the regular PulsarClient Builder API ?

It seems that this would just add unnecessary complexity and extra baggage in the public API.
I'd assume that there are very few use cases where there would be a need to specify the thread pools to use.

My use case is in improving nosqlbench's Pulsar driver. This would allow to emulate thousands of isolated Pulsar clients within a single JVM. When a Pulsar client is shared, it doesn't properly emulate real production work loads.

@Anonymitaet
Copy link
Member

@lhotari Thanks for your contribution. For this PR, do we need to update docs?

(The PR template contains info about doc, which helps others know more about the changes. Can you provide doc-related info in this and future PR descriptions? Thanks)

@eolivelli eolivelli requested a review from 315157973 September 21, 2021 13:44
@lhotari lhotari requested a review from merlimat September 23, 2021 12:34
@eolivelli eolivelli modified the milestones: 2.9.0, 2.10.0 Oct 6, 2021
@codelipenghui
Copy link
Contributor

@lhotari Could you please help resolve the conflicts? It's a nice have in 2.10.

@lhotari lhotari force-pushed the lh-support-passing-existing-executor-providers branch from 8a965f3 to 6a7f20c Compare January 18, 2022 08:36
@lhotari lhotari force-pushed the lh-support-passing-existing-executor-providers branch from 6a7f20c to b1ffa00 Compare January 18, 2022 08:40
@lhotari
Copy link
Member Author

lhotari commented Jan 18, 2022

@lhotari Could you please help resolve the conflicts? It's a nice have in 2.10.

@codelipenghui I have rebased the changes and addressed the review comments. PTAL

@lhotari lhotari requested a review from codelipenghui January 18, 2022 08:45
@lhotari
Copy link
Member Author

lhotari commented Jan 19, 2022

@codelipenghui This PR is now ready for final review

@codelipenghui codelipenghui merged commit 4591a21 into apache:master Jan 19, 2022
codelipenghui added a commit to codelipenghui/incubator-pulsar that referenced this pull request Jul 1, 2022
…oviders to the client

### Motivation

apache#16236 introduced a new scheduled executor but does not support passing the existing scheduled executor providers like apache#12037.
merlimat pushed a commit that referenced this pull request Jul 1, 2022
…oviders to the client (#16334)

* [improve][java-client] Support passing existing scheduled executor providers to the client

### Motivation

#16236 introduced a new scheduled executor but does not support passing the existing scheduled executor providers like #12037.

* Apply comment.
Nicklee007 pushed a commit to Nicklee007/pulsar that referenced this pull request Jul 5, 2022
…oviders to the client (apache#16334)

* [improve][java-client] Support passing existing scheduled executor providers to the client

### Motivation

apache#16236 introduced a new scheduled executor but does not support passing the existing scheduled executor providers like apache#12037.

* Apply comment.
codelipenghui added a commit that referenced this pull request Jul 10, 2022
…oviders to the client (#16334)

* [improve][java-client] Support passing existing scheduled executor providers to the client

### Motivation

#16236 introduced a new scheduled executor but does not support passing the existing scheduled executor providers like #12037.

* Apply comment.

(cherry picked from commit f159f62)
nicoloboschi pushed a commit to datastax/pulsar that referenced this pull request Jul 11, 2022
…oviders to the client (apache#16334)

* [improve][java-client] Support passing existing scheduled executor providers to the client

### Motivation

apache#16236 introduced a new scheduled executor but does not support passing the existing scheduled executor providers like apache#12037.

* Apply comment.

(cherry picked from commit f159f62)
(cherry picked from commit 4562d25)
wuxuanqicn pushed a commit to wuxuanqicn/pulsar that referenced this pull request Jul 14, 2022
…oviders to the client (apache#16334)

* [improve][java-client] Support passing existing scheduled executor providers to the client

### Motivation

apache#16236 introduced a new scheduled executor but does not support passing the existing scheduled executor providers like apache#12037.

* Apply comment.
@congbobo184 congbobo184 added the cherry-picked/branch-2.9 Archived: 2.9 is end of life label Nov 10, 2022
congbobo184 pushed a commit that referenced this pull request Nov 10, 2022
…12037)

In load and performance testing, there's a need to simulate production use cases and production workloads.
For this purpose, it would be useful to be able to share the thread pools used by Pulsar client instances in order to be able to run a large amount of Pulsar clients in a single JVM without the overhead of a lot of threads.
In the current solution, it's already possible to share the EventLoopGroup and HashedWheelTimer instances.
The solution for sharing the thread pools for the external / internal executors was missing. This PR adds support for that.

Example usage:

```java
// shared thread pool related resources
ExecutorProvider internalExecutorProvider = new ExecutorProvider(8, "shared-internal-executor");
ExecutorProvider externalExecutorProvider = new ExecutorProvider(8, "shared-external-executor");
Timer sharedTimer = new HashedWheelTimer(getThreadFactory("shared-pulsar-timer"), 1, TimeUnit.MILLISECONDS);
EventLoopGroup sharedEventLoopGroup = new EpollEventLoopGroup();

// example of creating a client which uses the shared thread pools
PulsarClientImpl client = PulsarClientImpl.builder().conf(conf)
                .internalExecutorProvider(internalExecutorProvider)
                .externalExecutorProvider(externalExecutorProvider)
                .timer(sharedTimer)
                .eventLoopGroup(sharedEventLoopGroup)
                .build();
```

It seems that this would also improve the performance of the Pulsar Proxy since new thread pools for every client connection.

That happens in the Pulsar Proxy currently:
https://github.com/apache/pulsar/blob/af63e96d4aaa0ae4c4086583aa4f9b1edd72279b/pulsar-proxy/src/main/java/org/apache/pulsar/proxy/server/ProxyConnection.java#L445-L451

An optimization was added in #9802 for sharing the timer, but it would be useful to also share the internal / external executors.

(cherry picked from commit 4591a21)
congbobo184 pushed a commit that referenced this pull request Nov 10, 2022
…oviders to the client (#16334)

* [improve][java-client] Support passing existing scheduled executor providers to the client

### Motivation

#16236 introduced a new scheduled executor but does not support passing the existing scheduled executor providers like #12037.

* Apply comment.

(cherry picked from commit f159f62)
congbobo184 pushed a commit that referenced this pull request Nov 26, 2022
…12037)

In load and performance testing, there's a need to simulate production use cases and production workloads.
For this purpose, it would be useful to be able to share the thread pools used by Pulsar client instances in order to be able to run a large amount of Pulsar clients in a single JVM without the overhead of a lot of threads.
In the current solution, it's already possible to share the EventLoopGroup and HashedWheelTimer instances.
The solution for sharing the thread pools for the external / internal executors was missing. This PR adds support for that.

Example usage:

```java
// shared thread pool related resources
ExecutorProvider internalExecutorProvider = new ExecutorProvider(8, "shared-internal-executor");
ExecutorProvider externalExecutorProvider = new ExecutorProvider(8, "shared-external-executor");
Timer sharedTimer = new HashedWheelTimer(getThreadFactory("shared-pulsar-timer"), 1, TimeUnit.MILLISECONDS);
EventLoopGroup sharedEventLoopGroup = new EpollEventLoopGroup();

// example of creating a client which uses the shared thread pools
PulsarClientImpl client = PulsarClientImpl.builder().conf(conf)
                .internalExecutorProvider(internalExecutorProvider)
                .externalExecutorProvider(externalExecutorProvider)
                .timer(sharedTimer)
                .eventLoopGroup(sharedEventLoopGroup)
                .build();
```

It seems that this would also improve the performance of the Pulsar Proxy since new thread pools for every client connection.

That happens in the Pulsar Proxy currently:
https://github.com/apache/pulsar/blob/af63e96d4aaa0ae4c4086583aa4f9b1edd72279b/pulsar-proxy/src/main/java/org/apache/pulsar/proxy/server/ProxyConnection.java#L445-L451

An optimization was added in #9802 for sharing the timer, but it would be useful to also share the internal / external executors.

(cherry picked from commit 4591a21)
congbobo184 pushed a commit that referenced this pull request Dec 1, 2022
…oviders to the client (#16334)

* [improve][java-client] Support passing existing scheduled executor providers to the client

### Motivation

#16236 introduced a new scheduled executor but does not support passing the existing scheduled executor providers like #12037.

* Apply comment.

(cherry picked from commit f159f62)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants