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

Allow to configure the number of BK client worker threads #10649

Merged
merged 1 commit into from
May 21, 2021

Conversation

merlimat
Copy link
Contributor

Motivation

In broker, we should be allowed to configure the number of threads to be be used for the BK worker pool.

@merlimat merlimat added the type/enhancement The enhancements for the existing features or docs. e.g. reduce memory usage of the delayed messages label May 20, 2021
@merlimat merlimat added this to the 2.8.0 milestone May 20, 2021
@merlimat merlimat self-assigned this May 20, 2021
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.

There is no need for this change, apart from documentation.

Since 2.8 (backported to 2.7.2)

Any configuration entry that starts with bookkeeper_ is passed to the BK client

@merlimat
Copy link
Contributor Author

We need to have this documented and visible though.

@eolivelli
Copy link
Contributor

Sorry I amended my comment.
I wanted to say that we just have to document this property.

@merlimat
Copy link
Contributor Author

I think that having that in broker.conf is the most appropriate form to advertise the configurability of such parameter.

@eolivelli
Copy link
Contributor

eolivelli commented May 20, 2021

I think that having that in broker.conf is the most appropriate form to advertise the configurability of such parameter.

Yes. We could add the line to broker.conf.
Nothing more

@merlimat
Copy link
Contributor Author

Having the bookkeeper_ prefix is good for settings that we don't expect to have to change in broker (while retaining the ability to do so), but for settings that are exposed directly in broker, we should follow the existing convention and naming scheme.

For this setting, I believe that it is very relevant to the performance of brokers and we should have it in same consistent way as the other BK client settings within broker.conf.

@sijie
Copy link
Member

sijie commented May 21, 2021

Having this setting available can emphasize its importance. Because it is very relevant to the performance. +1 on adding this setting.

@sijie
Copy link
Member

sijie commented May 21, 2021

@eolivelli

bookkeeper_ prefix is designed for allowing configuring bookkeeper client settings if they are not available in the broker configuration. But if there are settings critical for configuring bookkeeper client, we should make new settings for them, make them explicit in the broker configuration and follow the naming convention of the other bookkeeper client settings.

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.

Makes sense to me

+1

@eolivelli eolivelli merged commit 16223ba into apache:master May 21, 2021
@merlimat merlimat deleted the configure-bk-threads branch May 21, 2021 12:46
yangl pushed a commit to yangl/pulsar that referenced this pull request Jun 23, 2021
bharanic-dev pushed a commit to bharanic-dev/pulsar that referenced this pull request Mar 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type/enhancement The enhancements for the existing features or docs. e.g. reduce memory usage of the delayed messages
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants