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

feat[balance_strategy]: announcing a new round robin balance strategy #1788

Merged
merged 4 commits into from Dec 24, 2020

Conversation

kzinglzy
Copy link
Contributor

@kzinglzy kzinglzy commented Aug 23, 2020

There is a different behavior between

  • sarama's roundrobin balance strategy
     // pseudo code
     for topic, partitions := range allTopicPartitions {
     	i := 0
     	for partition := range allPartitions {
     		consumer := allconsumers[i % len(allconsumers)]
     		consumer.assign(topic, partition)
     		i += 1
     	}
     }
  • kafka java client's roundrobin balance strategy:
     // pseudo code
     i := 0
     for partition := range AllSortedPartitions {
     	consumer := allconsumers[i % len(allconsumers)]
     	consumer.assign(topic, partition)
     	i += 1
     }

In some case, sarama's implementation may incur skewed partitions,
for example:

// balance_strategy_test.go/TestBalanceStrategyRoundRobin:tests
{
    members: map[string][]string{"M": {"T1", "T2", "TT2"}, "M2": {"T1", "T2", "TT2"}, "M3": {"T1", "T2", "TT2"}},
    topics:  map[string][]int32{"T1": {0}, "T2": {0}, "TT2": {0}},
    expected: BalanceStrategyPlan{
        // there are three members and topics, however all the topics and partitions are assign to a single member
	"M": map[string][]int32{"T1": {0}, "T2": {0}, "TT2": {0}},
},

This PR provides the "really" roundrobin implementation that same as kafka java client did

@kzinglzy kzinglzy requested a review from bai as a code owner Aug 23, 2020
@kzinglzy kzinglzy force-pushed the even-round-robin-balance-strategy branch from be9fdc6 to 32d305d Compare Aug 23, 2020
dnwe
dnwe approved these changes Dec 14, 2020
Copy link
Collaborator

@dnwe dnwe left a comment

@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?

@kzinglzy
Copy link
Contributor Author

kzinglzy commented Dec 15, 2020

@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

so this PR should be considered as a bug fixing :P

@dnwe
Copy link
Collaborator

dnwe commented Dec 15, 2020

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.

Copy link
Contributor

@bai bai left a comment

I think it'd be OK to push this new strategy as a replacement (essentially a fix) and rename current one to Legacy.

@kzinglzy
Copy link
Contributor Author

kzinglzy commented Dec 23, 2020

@bai the original implementation has been replaced

Um... CI failed, looks like a test env issue?

@bai
Copy link
Contributor

bai commented Dec 24, 2020

Looks like a change in GitHub Actions broke CI. I've pushed a fix in #1862. Could you please rebase off of master?

@kzinglzy kzinglzy force-pushed the even-round-robin-balance-strategy branch from f5fe80e to 2caa4f7 Compare Dec 24, 2020
@kzinglzy
Copy link
Contributor Author

kzinglzy commented Dec 24, 2020

@bai please take a look, ready for merge

bai
bai approved these changes Dec 24, 2020
@bai bai merged commit 0cdf4a6 into Shopify:master Dec 24, 2020
3 checks passed
@kzinglzy kzinglzy deleted the even-round-robin-balance-strategy branch Dec 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants