Allow config of IO and acceptor threads in proxy #14054
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Previously, the Pulasr Proxy did not allow configuration of the number of IO threads and acceptor threads in the proxy. These options can be very important to tune, as is tuneable in the broker, so this change simply matches the brokers perspective. Also, we increase the default number of IO threads to 2x number of processors instead of 1x, as in a single CPU config, it still makes sense to have 2 threads, at least for now, where some blocking operatings can happen (such as authn/authz plugins)
|
@addisonj:Thanks for your contribution. For this PR, do we need to update docs? |
codelipenghui
approved these changes
Jan 30, 2022
codelipenghui
added
type/enhancement
hangc0276
reviewed
Jan 30, 2022
| this.workerGroup = EventLoopUtil.newEventLoopGroup(numThreads, false, workersThreadFactory); | ||
| this.acceptorGroup = EventLoopUtil.newEventLoopGroup(proxyConfig.getNumAcceptorThreads(), | ||
| false, acceptorThreadFactory); | ||
| this.workerGroup = EventLoopUtil.newEventLoopGroup(proxyConfig.getNumIOThreads(), |
There was a problem hiding this comment.
The previous default numThreads is Runtime.getRuntime().availableProcessors(), but the current default numThreads is Runtime.getRuntime().availableProcessors(). It has break the default behavior. Do we need to keep the default value?
There was a problem hiding this comment.
I think we should move to the same default as the broker... primarily because I don't think having a single IO thread is generally something we want to do.
lhotari
pushed a commit
to lhotari/pulsar
that referenced
this pull request
Feb 9, 2022
* Allow config of IO and acceptor threads in proxy Previously, the Pulasr Proxy did not allow configuration of the number of IO threads and acceptor threads in the proxy. These options can be very important to tune, as is tuneable in the broker, so this change simply matches the brokers perspective. Also, we increase the default number of IO threads to 2x number of processors instead of 1x, as in a single CPU config, it still makes sense to have 2 threads, at least for now, where some blocking operatings can happen (such as authn/authz plugins) * fix checkstyle (cherry picked from commit f455418)
lhotari
pushed a commit
that referenced
this pull request
Feb 9, 2022
* Allow config of IO and acceptor threads in proxy Previously, the Pulasr Proxy did not allow configuration of the number of IO threads and acceptor threads in the proxy. These options can be very important to tune, as is tuneable in the broker, so this change simply matches the brokers perspective. Also, we increase the default number of IO threads to 2x number of processors instead of 1x, as in a single CPU config, it still makes sense to have 2 threads, at least for now, where some blocking operatings can happen (such as authn/authz plugins) * fix checkstyle (cherry picked from commit f455418)
lhotari
pushed a commit
to datastax/pulsar
that referenced
this pull request
Feb 9, 2022
* Allow config of IO and acceptor threads in proxy Previously, the Pulasr Proxy did not allow configuration of the number of IO threads and acceptor threads in the proxy. These options can be very important to tune, as is tuneable in the broker, so this change simply matches the brokers perspective. Also, we increase the default number of IO threads to 2x number of processors instead of 1x, as in a single CPU config, it still makes sense to have 2 threads, at least for now, where some blocking operatings can happen (such as authn/authz plugins) * fix checkstyle (cherry picked from commit f455418) (cherry picked from commit 1742df8)
Nicklee007
pushed a commit
to Nicklee007/pulsar
that referenced
this pull request
Apr 20, 2022
* Allow config of IO and acceptor threads in proxy Previously, the Pulasr Proxy did not allow configuration of the number of IO threads and acceptor threads in the proxy. These options can be very important to tune, as is tuneable in the broker, so this change simply matches the brokers perspective. Also, we increase the default number of IO threads to 2x number of processors instead of 1x, as in a single CPU config, it still makes sense to have 2 threads, at least for now, where some blocking operatings can happen (such as authn/authz plugins) * fix checkstyle
lhotari
pushed a commit
to datastax/pulsar
that referenced
this pull request
Apr 20, 2022
* Allow config of IO and acceptor threads in proxy Previously, the Pulasr Proxy did not allow configuration of the number of IO threads and acceptor threads in the proxy. These options can be very important to tune, as is tuneable in the broker, so this change simply matches the brokers perspective. Also, we increase the default number of IO threads to 2x number of processors instead of 1x, as in a single CPU config, it still makes sense to have 2 threads, at least for now, where some blocking operatings can happen (such as authn/authz plugins) * fix checkstyle (cherry picked from commit f455418)
Anonymitaet
added
doc-required
|
@addisonj When submitting a PR, can you provide doc-related info (tick the box) in the PR description? So that Bot can recognize and label your PR correspondingly. Or else Bot labels your PR with |
|
@momo-jun a soft reminder: here is a PR w/ doc-required label, could u pls follow up? Thanks |
4 tasks
|
@Anonymitaet, this PR requires changes to both config files and docs. @Demogorgon314 has taken care of it. |
Anonymitaet
added
doc-complete
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation
Previously, the Pulasr Proxy did not allow configuration of the number
of IO threads and acceptor threads in the proxy.
These options can be very important to tune, as is tuneable in the
broker, so this change simply matches the brokers perspective.
Modifications
This adds the options to tune threads to the proxy.
It also increases the default number of IO threads to 2x number of
processors instead of 1x, as in a single CPU config, it still makes
sense to have 2 threads, at least for now, where some blocking
operatings can happen (such as authn/authz plugins)
Verifying this change
This change is a trivial rework / code cleanup without any test coverage.
Does this pull request potentially affect one of the following parts:
If
yeswas chosen, please highlight the changesDocumentation
Javadoc is updated, but reference docs need updated as well