-
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] [pip] PIP-364: Introduce a new load balance algorithm AvgShedder #22946
Conversation
PTAL, thanks! There is a lot of content, hope them will be helpful to you. |
Impressive work @thetumbled and team! |
I am curious if we can compare this with the latest shedder, TransferShedder. |
|
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.
Great work!
category = CATEGORY_LOAD_BALANCER, | ||
doc = "In the UniformLoadShedder strategy, the minimum throughput that triggers unload." | ||
) | ||
private int minUnloadMessageThroughput = 1 * 1024 * 1024; |
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 would be great if we could make the minUnloadMessageThroughput calculated dynamically. This idea aims to resolve the following point.
When a Pulsar cluster has a huge number of topics/producers/consumers, but with low traffic, it is hard for us to determine the minUnloadMessageThroughput
. If we change this value too small, and new traffic comes in, the topics will be transferred between brokers with high frequency.
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.
Actually, AvgShedder
can deal with such situation without minUnloadMessageThroughput
.
The design of AvgShedder
comes with high stability.
I assume you refer to this part.
Actually, TransferShedder is trying to provide correct load data to the leader by
|
yes.
It may relieve the defect of historical scoring algorithm, but i am worried that such kind of improvement will bind the algorithm with the load manager together. Maybe we will implement the AvgShedder in the new load manager, right? Actually, historical scoring algorithm introduce too much complexity into load balance module, and the benefit it bring is not that significant. When a wrong shedding decision is made, it is hard to debug why it decide to do that. The improvement you design will introduce much more complexity. I prefer to introduce AvgShedder into new load manager first, WDYT? @Demogorgon314 @heesung-sn |
You meant the modular load manager here? Yes, AvgShedder is improvement in modular load manager, indeed. |
No, I means we can try to introduce
|
It would be great if the community starts testing/improving ExtensibleLoadManager. |
Implementation PR: #22949
Motivation
Current load balance algo has defect.
Modifications
Introduce a new load balance algorithm AvgShedder.
Verifying this change
(Please pick either of the following options)
This change is a trivial rework / code cleanup without any test coverage.
Does this pull request potentially affect one of the following parts:
If the box was checked, please highlight the changes
Documentation
doc
doc-required
doc-not-needed
doc-complete
Matching PR in forked repository
PR in forked repository: