-
Notifications
You must be signed in to change notification settings - Fork 3.7k
[improve][broker] Reuse io eventloop and other executors for broker side PulsarAdminImpl
#20460
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
[improve][broker] Reuse io eventloop and other executors for broker side PulsarAdminImpl
#20460
Conversation
…s created on the broker side.
|
|
||
| /** | ||
| * This override the pulsar admin client | ||
| * netty EventLoopGroup when admin is created on the broker side. | ||
| * @param eventLoopGroup | ||
| * @return this | ||
| */ | ||
| PulsarAdminBuilder setEventLoopGroup(EventLoopGroup eventLoopGroup); | ||
|
|
||
| /** | ||
| * This override the pulsar admin client | ||
| * netty Timer when admin is created on the broker side. | ||
| * @param nettyTimer | ||
| * @return this | ||
| */ | ||
| PulsarAdminBuilder setNettyTimer(Timer nettyTimer); | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This cannot be exposed on the public API. It will cause issues on the shaded client since the Netty packages are shaded. The decision has been to hide this type of details from the public API.
Please see PIP-234 discussion about this: https://lists.apache.org/thread/5jw06hqlmwnrgvbn9lfom1vkwhwqwwd4 .
A temporary solution is to make this possible in the private API such as has been done for the Pulsar Client currently. Hopefully we eventually find a solution for PIP-234. The design hasn't been completed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
another thread is https://lists.apache.org/thread/5obfm17g58n3dnbzyxg57vokgmwyp6hx
|
@lhotari Thank you so much for providing background links. I hide those share resource method in the |
80270b9 to
fc54774
Compare
…zzlefun/pulsar into reuse_timer_and_eventloop
PulsarAdminImplPulsarAdminImpl
|
all unit test pass in my local repo except coverage upload see lifepuzzlefun#19 |
757b0e2 to
e3249dd
Compare
|
@lhotari Hi, I have changed the code, PTAL : -) |
|
The pr had no activity for 30 days, mark with Stale label. |
|
Addressed by PIP-234 related #24893. This isn't currently used in the broker, but that could be addressed in another PR if it's really needed. |
Motivation
Current when PulsarAdmin is create on the broker side. the internal async http client will create its own netty eventloop, netty timer and one ScheduledExecutorService. maybe we can just reuse the broker side resources like PulsarClient.
Modifications
SharedExecutorContextto save eventloop, timer, and schedulerExecutor.PulsarAdminBuilderImpl.AsyncHttpConnectorProviderAsyncHttpClientAsyncHttpClienthas its own logic to check if the eventloop and timer is provided by the api callerwhich prevent those object to be closed.
Verifying this change
This change is already covered by existing tests, such as (please describe tests).
Does this pull request potentially affect one of the following parts:
If the box was checked, please highlight the changes
Documentation
docdoc-requireddoc-not-neededdoc-completeMatching PR in forked repository
PR in forked repository: lifepuzzlefun#19