-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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] PIP-192: Support broker isolation policy #19592
[improve][broker] PIP-192: Support broker isolation policy #19592
Conversation
4175af3
to
9404c68
Compare
...src/main/java/org/apache/pulsar/broker/loadbalance/extensions/scheduler/TransferShedder.java
Outdated
Show resolved
Hide resolved
...src/main/java/org/apache/pulsar/broker/loadbalance/extensions/scheduler/TransferShedder.java
Outdated
Show resolved
Hide resolved
...n/java/org/apache/pulsar/broker/loadbalance/extensions/policies/IsolationPoliciesHelper.java
Outdated
Show resolved
Hide resolved
} | ||
}; | ||
|
||
public Set<String> applyIsolationPolicies(Map<String, BrokerLookupData> availableBrokers, |
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.
It is good to consider the isolation policy here in this shedding strategy. However, it might be better if we just do not automatically unload/transfer bundles that configure any isolation policy or anti-affinity group.
Reasoning: These bundles configure limited sets of brokers to transfer/unload, which is not an ideal target to move around regarding load balance. (Hopefully, not all of the top k loaded bundles comply with isolation policy) It would be better to move other bundles that don't comply with any policies. "Fix bundles that have hard limits but move other free ones."
For this reason, we could probably filter out those bundles with isolation or anti-affinity group policy in the TopKBundles,
just like we filter out the system namespace bundles. WDYT?
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.
Good idea!
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.
I don't think it is harmful to keep this logic here. I can add those bundle filtering logic(filtering out bundles with isolation and anti-affinity-group in topk bundles) in my next PR(anti-affinity group support).
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.
Yes, let's keep the logic here temporarily, since your proposal will change the original behavior. WDYT?
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.
Sure. sounds good to me.
...src/main/java/org/apache/pulsar/broker/loadbalance/extensions/scheduler/TransferShedder.java
Show resolved
Hide resolved
1c2e004
to
aaec1c8
Compare
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.
LGTM
/pulsarbot run-failure-checks |
Master Issue: #16691
Motivation
Currently, The
ExtensibleLoadManager
don't support broker isolation policy yet.Modifications
Add a
BrokerIsolationPoliciesFilter
to filter out an unqualified broker with isolation policies.Add broker isolation support for
TransferShedder
.Documentation
doc
doc-required
doc-not-needed
doc-complete