[feat][broker] Support lower boundary shedding for ThresholdShedder - #17456
Conversation
# Conflicts: # pulsar-broker/src/main/java/org/apache/pulsar/broker/loadbalance/impl/ThresholdShedder.java
github-actions
Bot
added
the
doc-required
eolivelli
left a comment
eolivelli
left a comment
There was a problem hiding this comment.
Why don’t we simply fix the existing class. This new behaviour seems better.
|
We are not sure that the new way completely fine, ThresholdShedder is now the default algorithm, if there are some problems, it will affect the existing business, so we'd better run separately. |
|
As the description of the issue, this appears to be an obvious flaw of the current |
As I said above, there is no way to guarantee that this new feature will be fully stable, and if something goes wrong, we can't even turn it off. |
Technoboy-
added
type/feature
Technoboy-
changed the title
hangc0276
left a comment
hangc0276
left a comment
There was a problem hiding this comment.
Good job!
This is an improvement of our current ThresholdShedder. I prefer to integrate this improvement into ThresholdSheddger with a flag to turn this feature on.
By default, we can turn it off, after it becomes stable, we can turn it on by default.
eolivelli
left a comment
eolivelli
left a comment
There was a problem hiding this comment.
This is the core of the question "once it is considered stable".
Who is in charge of doing this testing ?
Choosing between this implementation and the original one is something that nobody would ever do in production.
If you (we) think that this is a bug fix then we have to push it.
We have more that 3 months to see this working on master branch before cutting a new major release (2.12).
|
@eolivelli PTAL |
Please help CR, I will verify it in production environment |
michaeljmarshall
left a comment
michaeljmarshall
left a comment
There was a problem hiding this comment.
I think this addition makes sense as a configuration to modify the ThresholdShedder. I left some comments. Please also include an update to the ThresholdShedder Javadoc so that it is up to date @315157973.
…rviceConfiguration.java Co-authored-by: Michael Marshall <mikemarsh17@gmail.com>
codelipenghui
left a comment
codelipenghui
left a comment
There was a problem hiding this comment.
Others looks good to me.
| private Pair<Boolean, String> getMaxUsageBroker( | ||
| LoadData loadData, double threshold, double avgUsage) { | ||
| String maxUsageBrokerName = ""; | ||
| double maxUsage = avgUsage - threshold; |
There was a problem hiding this comment.
The max usage broker load: > (avgUsage - threshold)
The below lower broker load: < (avgUsage - threshold)
After the max usage broker unloads to the lower broker, the max usage broker might become the lower broker, the lower broker becomes the max usage broker. Can this will lead to frequent bundle unloading?
It looks like we need to change
double minimumThroughputToOffload = brokerCurrentThroughput * threshold * LOWER_BOUNDARY_THRESHOLD_MARGIN;to
double minimumThroughputToOffload = Math.min(brokerCurrentThroughput * threshold * LOWER_BOUNDARY_THRESHOLD_MARGIN, brokerCurrentThroughput - avgUsage - threshold);There was a problem hiding this comment.
And we should also add a test for this case.
There was a problem hiding this comment.
It does not solve the problem.
Unless we know which Bundle is to be unloaded and the load of the Bundle, we can determine whether to select the current Broker. Currently, this Class is only responsible for selecting brokers and cannot know the information about bundles to be unloaded.
The best way to solve this problem is to split bundle.
…ce/impl/ThresholdShedderTest.java Co-authored-by: Penghui Li <penghui@apache.org>
…ce/impl/ThresholdShedderTest.java Co-authored-by: Penghui Li <penghui@apache.org>
…ce/impl/ThresholdShedderTest.java Co-authored-by: Penghui Li <penghui@apache.org>
|
@315157973 Please also update the PR title, it should be an improvement for the threshold shedder |
315157973
changed the title
eolivelli
left a comment
eolivelli
left a comment
There was a problem hiding this comment.
LGTM
good idea
@dave2wave @MMirelli FYI
|
@315157973 It will help users understand this feature if you can add the related docs. Do you have any planned updates on that? |
I'll submit the docs soon. |
…17456) Support lower boundary shedding for ThresholdShedder (#17456) The existing ThresholdShedder has the following problems, for example: There are 11 Brokers, of which 10 are loaded at 80% and 1 is loaded at 0%. The average load is 80 * 10 / 11 = 72.73, and the threshold to unload is 72.73 + 10 = 82.73. Since 80 < 82.73, unload will not be trigger, and there is one idle Broker with load of 0%. On the basis of ThresholdShedder, we adds the lower boundary judgment of the load. When 【current usage < average usage - threshold】, the broker with the highest load will be triggered to unload
@315157973 any updates on the docs? |
momo-jun
added
doc-complete
10 participants
Motivation
The existing ThresholdShedder has the following problems, for example:
There are 11 Brokers, of which 10 are loaded at 80% and 1 is loaded at 0%.
The average load is 80 * 10 / 11 = 72.73, and the threshold to unload is 72.73 + 10 = 82.73.
Since 80 < 82.73, unload will not be trigger, and there is one idle Broker with load of 0%.
On the basis of ThresholdShedder, we adds the lower boundary judgment of the load.
When 【current usage < average usage - threshold】, the broker with the highest load will be triggered to unload
Modifications
Support lower boundary shedding for ThresholdShedder
Verifying this change
Documentation
doc-required(Your PR needs to update docs and you will update later)