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
Merged

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

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 August 23, 2020 08:16
@ghost ghost added cla-needed and removed cla-needed labels Aug 23, 2020
Copy link
Collaborator

@dnwe dnwe left a 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?

@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

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.

@kzinglzy
Copy link
Contributor Author

@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
Copy link
Contributor Author

@bai please take a look, ready for merge

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