-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
feat[balance_strategy]: announcing a new round robin balance strategy #1788
Conversation
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.
@kzinglzy I'm happy to approve the changes, although I do wonder if the original implementation of the round robin strategy should just be considered flawed and we should replace it with your correct implementation rather than adding this as a new alternative strategy? Whilst I know it would technically be a potentially unexpected change of behaviour for people, it still seems like the right thing to do.
@bai thoughts? I wonder if we should rename the old strategy to something with "legacy" in the name and then make @kzinglzy's implementation the "RoundRobinBalancer" and just mention the change of behaviour in the next release notes?
as mentioned previously, the old implementation of round robin strategy is not so "standard" , in some cases even wrong (e.g. skewed partitions), thus, it may be ok to replace it with a more "standard" implementation directly
|
Yeah that was my thinking. To my reading our original implementation was just incorrect, I can't think of a good reason why anyone would prefer it and I think we should accept your PR as a replacement that corrects the behaviour to match the original intention. |
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 think it'd be OK to push this new strategy as a replacement (essentially a fix) and rename current one to Legacy.
@bai the original implementation has been replaced
|
Looks like a change in GitHub Actions broke CI. I've pushed a fix in #1862. Could you please rebase off of master? |
@bai please take a look, ready for merge |
There is a different behavior between
In some case, sarama's implementation may incur skewed partitions,
for example:
This PR provides the "really" roundrobin implementation that same as kafka java client did