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

[pulsar-broker] Make load-balancer config dynamic for the runtime tuning #13074

Merged
merged 2 commits into from
Jan 8, 2022

Conversation

rdhabalia
Copy link
Contributor

Motivation

There are a few load balancer config that requires tuning at runtime without restarting the broker. so, mark those configs dynamic so, admin can tune them based on cluster requirements.

@github-actions
Copy link

github-actions bot commented Dec 1, 2021

@rdhabalia: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)

@@ -1722,6 +1723,7 @@

@FieldContext(
category = CATEGORY_LOAD_BALANCER,
dynamic = true,
doc = "load balance load shedding strategy"
)
private String loadBalancerLoadSheddingStrategy = "org.apache.pulsar.broker.loadbalance.impl.OverloadShedder";
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems this loadBalancerLoadSheddingStrategy is used on broker start, dynamic won't take effect.
Please check if all these configs are only used on broker start up.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we would like to change config without deploying new code and dynamic config helps in that case. for a safe deployment we would like keep existing config, change config dynamically and do process restart without redeploying new code. so, this is an intentional code change.

Copy link
Contributor

Choose a reason for hiding this comment

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

I get your point. But is a little confusing, if we just mark it dynamic, but we need to restart to make it take effect.

Most importantly, we need to tell the difference that which of these dynamic configs need broker restart, and which ones don't.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, we can add into doc. I will add it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

Copy link
Contributor

Choose a reason for hiding this comment

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

Only loadBalancerLoadSheddingStrategy needs restart? Have you checked others?

@eolivelli
Copy link
Contributor

@rdhabalia can you please give me some pointers about how "dynamic" works ?

@rdhabalia
Copy link
Contributor Author

@eolivelli dynamic config provides a way to change broker-config value dynamically using admin-api and it also persists those changed values and applies them on broker server startup. this feature is useful when you want to tune broker at runtime or experiment any value without deploying new release of changes.

@rdhabalia
Copy link
Contributor Author

ping.

@Jason918
Copy link
Contributor

change config without deploying new code

I think it's a common require and we can add this as default feature for most of the configs.
As for dynamic, IMHO, it's better to keep it to annotate the configs that can be update and take effect online.

@rdhabalia
Copy link
Contributor Author

I think we kept most of them dynamic which are required.
However, can we merge this PR as it's been open for long and time require to tune load balancer.

@rdhabalia
Copy link
Contributor Author

ping. can someone please review the PR?

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.

+1

@eolivelli eolivelli merged commit 931194c into apache:master Jan 8, 2022
liudezhi2098 pushed a commit to liudezhi2098/pulsar that referenced this pull request Jan 11, 2022
…ing (apache#13074)

* [pulsar-broker] Make load-balancer config dynamic for the runtime tuning

* add doc for dynamic restart
@Anonymitaet
Copy link
Member

@momo-jun is doc updated for this PR? Can you help check? Thanks

@Anonymitaet Anonymitaet added doc-complete Your PR changes impact docs and the related docs have been already added. and removed doc-label-missing labels Apr 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/broker doc-complete Your PR changes impact docs and the related docs have been already added.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants