[ISSUE 9783][pulsar-client] Allow pulsar client receive external timer #9802

linlinnn · 2021-03-04T09:23:36Z

Fixed #9783

Motivation

Allow pulsar client to receive external timer instance

Modifications

Add new constructor to provide an external timer, and share timer in pulsar proxy

Verifying this change

Make sure that the change passes the CI checks.

eolivelli

I am not sure this is the right approach.
Probably it would be better to allow the PulsarClient to receive an external instance of the timer

How many PulsarClient instances do you have in your application?

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

codelipenghui · 2021-03-04T13:18:56Z

@eolivelli Make sense, receive an external instance is a better choice.

codelipenghui

It needs to add some tests to cover the changes.

linlinnn · 2021-03-05T02:27:33Z

Is that means to we need to refact the constructor of the PulsarClient? or take a stratege when there are so many PulsarClient instances that over the threshold.

codelipenghui · 2021-03-05T03:09:10Z

@linlinnn No, we can add a new method for the Client Builder such as .timer(Timer timer) to allow users to provide the HashedWheelTimer, so that users can decide how to create the HashedWheelTimer and when to close it.

linlinnn · 2021-03-05T05:30:04Z

@codelipenghui thanks, get it.

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

eolivelli

LGTM

pulsar-client-api/src/main/java/org/apache/pulsar/client/api/ClientBuilder.java

codelipenghui · 2021-03-15T12:09:57Z

@linlinnn Are you working on this PR?

linlinnn · 2021-03-16T07:31:52Z

@codelipenghui PTAL, I will add test cases soon if this PR is reasonable

eolivelli

I don't find a way for the user to use this feature.
Users use the Client Builder and they must not use the public constructors directly.

Can you please add support for the timer to the builder ?

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

codelipenghui · 2021-03-18T07:25:28Z

@eolivelli Could you please check the previous comments? I think there is a discussion that @merlimat brings up to discuss why do not expose it to the ClientBuilder.

eolivelli · 2021-03-18T09:15:23Z

@codelipenghui @linlinnn
@merlimat comment is

Instead, we could take a Timer instance from the PulsarClientImpl constructor and have the Pulsar proxy to pass it there.

now the patch is only adding a way to pass a Timer to the PulsarClientImpl.

I believe that the comment from Matteo is that is it okay to add a new constructor to PulsarClientImpl as far as it is only used in Pulsar codebase.
This new constructor is not meant to be used by users (by code outside Pulsar internals) but it is not used by Pulsar itelft.

I expect this patch to change Pulsar Proxy in order to use the new constructor.

codelipenghui · 2021-03-18T09:49:34Z

@eolivelli Yes, so this PR is trying to allow pass the external timer to the client, we can fix the proxy side in a separate PR? @linlinnn I think we can update the comment? this one can't fix #9783 totally, we need to create a separate to fix the proxy side.

linlinnn · 2021-03-19T06:02:01Z

/pulsarbot run-failure-checks

linlinnn · 2021-03-21T04:30:15Z

@merlimat please help review again

codelipenghui · 2021-03-22T03:18:06Z

ping @merlimat Please help review this PR again.

codelipenghui · 2021-03-25T00:56:49Z

ping @merlimat Please help review this PR again.

linlinnn · 2021-03-25T10:24:13Z

@codelipenghui @eolivelli Please take a look, I add share timer in pulsar proxy

eolivelli

good progress!

I left one suggestion

pulsar-proxy/src/main/java/org/apache/pulsar/proxy/server/ProxyConnection.java

pulsar-proxy/src/main/java/org/apache/pulsar/proxy/server/ProxyService.java

eolivelli

Looks great to me!

thank you very much

@merlimat PTAL

merlimat

👍

linlinnn · 2021-03-25T17:09:43Z

@codelipenghui @eolivelli @merlimat Thanks for your review.

linlinnn · 2021-03-26T01:30:19Z

/pulsarbot run-failure-checks

linlinnn · 2021-03-29T02:05:30Z

ping @codelipenghui PTAL

…nal timer (#9802)" This reverts commit af6eaba.

#9802) Fixed #9783 Allow pulsar client to receive external timer instance Add new constructor to provide an external timer, and share timer in pulsar proxy - [x] Make sure that the change passes the CI checks. (cherry picked from commit af6eaba)

…12037) ### 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: ```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.

lhotari · 2022-01-19T13:34:15Z

It turns out the PulsarClientImpl isn't needed at all in Pulsar Proxy. There's a PR #13836 to optimize the resource usage of Pulsar Proxy. This reduces 2 threads per proxied connection which will help improve Pulsar Proxy scalability. Please review #13836

…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)

eolivelli requested changes Mar 4, 2021

View reviewed changes

pulsar-client/src/main/java/org/apache/pulsar/client/impl/PulsarClientImpl.java Outdated Show resolved Hide resolved

codelipenghui reviewed Mar 4, 2021

View reviewed changes

codelipenghui assigned linlinnn Mar 4, 2021

codelipenghui added the type/enhancement The enhancements for the existing features or docs. e.g. reduce memory usage of the delayed messages label Mar 4, 2021

codelipenghui added this to the 2.8.0 milestone Mar 4, 2021

codelipenghui added the release/2.7.2 label Mar 4, 2021

codelipenghui requested a review from 315157973 March 4, 2021 13:20

codelipenghui unassigned linlinnn Mar 4, 2021

codelipenghui requested review from hangc0276, jiazhai, sijie, zymap and merlimat March 4, 2021 13:20

codelipenghui reviewed Mar 8, 2021

View reviewed changes

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

eolivelli approved these changes Mar 8, 2021

View reviewed changes

merlimat requested changes Mar 8, 2021

View reviewed changes

pulsar-client-api/src/main/java/org/apache/pulsar/client/api/ClientBuilder.java Outdated Show resolved Hide resolved

linlinnn force-pushed the issue-9783 branch from e760fbe to aa39c43 Compare March 16, 2021 07:25

add timer to PulsarClientImpl constructor

ee8ac01

linlinnn force-pushed the issue-9783 branch from aa39c43 to ee8ac01 Compare March 16, 2021 07:29

eolivelli requested changes Mar 16, 2021

View reviewed changes

pulsar-client/src/main/java/org/apache/pulsar/client/impl/PulsarClientImpl.java Outdated Show resolved Hide resolved

linlinnn requested a review from codelipenghui March 18, 2021 01:57

codelipenghui approved these changes Mar 19, 2021

View reviewed changes

enable share timer in pulsar proxy

f52516a

eolivelli requested changes Mar 25, 2021

View reviewed changes

pulsar-proxy/src/main/java/org/apache/pulsar/proxy/server/ProxyConnection.java Outdated Show resolved Hide resolved

pulsar-proxy/src/main/java/org/apache/pulsar/proxy/server/ProxyConnection.java Outdated Show resolved Hide resolved

revert

db2b41c

linlinnn requested review from eolivelli, codelipenghui and 315157973 March 25, 2021 14:17

eolivelli reviewed Mar 25, 2021

View reviewed changes

rename timer

b6b47cb

eolivelli approved these changes Mar 25, 2021

View reviewed changes

merlimat approved these changes Mar 25, 2021

View reviewed changes

codelipenghui merged commit af6eaba into apache:master Mar 29, 2021

codelipenghui added a commit that referenced this pull request Mar 29, 2021

Revert "[ISSUE 9783][pulsar-client] Allow pulsar client receive exter…

80e03c0

…nal timer (#9802)" This reverts commit af6eaba.

codelipenghui added the cherry-picked/branch-2.7 Archived: 2.7 is end of life label Mar 30, 2021

lhotari mentioned this pull request Sep 14, 2021

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

Merged

lhotari mentioned this pull request Jan 19, 2022

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

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ISSUE 9783][pulsar-client] Allow pulsar client receive external timer #9802

[ISSUE 9783][pulsar-client] Allow pulsar client receive external timer #9802

linlinnn commented Mar 4, 2021 •

edited

Loading

eolivelli left a comment

codelipenghui commented Mar 4, 2021

codelipenghui left a comment

linlinnn commented Mar 5, 2021

codelipenghui commented Mar 5, 2021

linlinnn commented Mar 5, 2021

eolivelli left a comment

codelipenghui commented Mar 15, 2021

linlinnn commented Mar 16, 2021

eolivelli left a comment

codelipenghui commented Mar 18, 2021

eolivelli commented Mar 18, 2021

codelipenghui commented Mar 18, 2021

linlinnn commented Mar 19, 2021

linlinnn commented Mar 21, 2021

codelipenghui commented Mar 22, 2021

codelipenghui commented Mar 25, 2021

linlinnn commented Mar 25, 2021

eolivelli left a comment

eolivelli left a comment

merlimat left a comment

linlinnn commented Mar 25, 2021

linlinnn commented Mar 26, 2021

linlinnn commented Mar 29, 2021

lhotari commented Jan 19, 2022

[ISSUE 9783][pulsar-client] Allow pulsar client receive external timer #9802

[ISSUE 9783][pulsar-client] Allow pulsar client receive external timer #9802

Conversation

linlinnn commented Mar 4, 2021 • edited Loading

Motivation

Modifications

Verifying this change

eolivelli left a comment

Choose a reason for hiding this comment

codelipenghui commented Mar 4, 2021

codelipenghui left a comment

Choose a reason for hiding this comment

linlinnn commented Mar 5, 2021

codelipenghui commented Mar 5, 2021

linlinnn commented Mar 5, 2021

eolivelli left a comment

Choose a reason for hiding this comment

codelipenghui commented Mar 15, 2021

linlinnn commented Mar 16, 2021

eolivelli left a comment

Choose a reason for hiding this comment

codelipenghui commented Mar 18, 2021

eolivelli commented Mar 18, 2021

codelipenghui commented Mar 18, 2021

linlinnn commented Mar 19, 2021

linlinnn commented Mar 21, 2021

codelipenghui commented Mar 22, 2021

codelipenghui commented Mar 25, 2021

linlinnn commented Mar 25, 2021

eolivelli left a comment

Choose a reason for hiding this comment

eolivelli left a comment

Choose a reason for hiding this comment

merlimat left a comment

Choose a reason for hiding this comment

linlinnn commented Mar 25, 2021

linlinnn commented Mar 26, 2021

linlinnn commented Mar 29, 2021

lhotari commented Jan 19, 2022

linlinnn commented Mar 4, 2021 •

edited

Loading