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

Configure backward compatibility on Group.Rebalance.GroupStrategies #2351

Closed
wathenjiang opened this issue Sep 30, 2022 · 4 comments
Closed

Comments

@wathenjiang
Copy link
Contributor

In d66826a commit, new configuration options Group.Rebalance.GroupStrategies have been introduced.

But it is not backward compatibility.

If I do the below starategy set in my older code, but use the latest sarama package, will encounter Consumer.Group.Rebalance.GroupStrategies and Consumer.Group.Rebalance.Strategy cannot be set at the same time error. Please see https://github.com/Shopify/sarama/blob/v1.37.0/config.go#L808

// config setting in my older setting
kconfig.Consumer.Group.Rebalance.Strategy = sarama.BalanceStrategyRoundRobin

That is because sarama always set a default value on Group.Rebalance.GroupStrategies. Please see https://github.com/Shopify/sarama/blob/v1.37.0/config.go#L542

The code is very confusing, util I read d66826a commit. Does it a violated Object-oriented design pattern? The Config should not exposed, so set Consumer.Group.Rebalance.Strategy and Consumer.Group.Rebalance.GroupStrategies can be atomic.
Just setting Consumer.Group.Rebalance.Strategy Deprecated is not enough.

@wathenjiang
Copy link
Contributor Author

wathenjiang commented Sep 30, 2022

By default sarama will not set https://github.com/Shopify/sarama/issues/2351, but set c.Consumer.Group.Rebalance.GroupStrategies.
If c.Consumer.Group.Rebalance.Strategy != nil is true, must set by user, user should have a higher priority than default setting. Or we might just delete the c.Consumer.Group.Rebalance.Strategy config.

See #2352.

@napallday
Copy link
Contributor

My bad. I think we can just delete this validation

case c.Consumer.Group.Rebalance.Strategy != nil && len(c.Consumer.Group.Rebalance.GroupStrategies) != 0:
	return ConfigurationError("Consumer.Group.Rebalance.GroupStrategies and Consumer.Group.Rebalance.Strategy cannot be set at the same time")

to make it compatible.

Because c.Consumer.Group.Rebalance.Strategy takes precedence of c.Consumer.Group.Rebalance.GroupStrategies.
wdyt?

@wathenjiang
Copy link
Contributor Author

My bad. I think we can just delete this validation

case c.Consumer.Group.Rebalance.Strategy != nil && len(c.Consumer.Group.Rebalance.GroupStrategies) != 0:
	return ConfigurationError("Consumer.Group.Rebalance.GroupStrategies and Consumer.Group.Rebalance.Strategy cannot be set at the same time")

to make it compatible.

Because c.Consumer.Group.Rebalance.Strategy takes precedence of c.Consumer.Group.Rebalance.GroupStrategies. wdyt?

I does not know the internal mechanism of saram well, If delete it has no side effect, we could delete the above case segment.
May u or matainer do code review on #2352?

@dnwe
Copy link
Collaborator

dnwe commented Oct 4, 2022

Fixed by #2352 — thanks for raising this

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

No branches or pull requests

3 participants