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

sarama-compatible hasher #622

Merged
merged 2 commits into from
Jan 21, 2024
Merged

sarama-compatible hasher #622

merged 2 commits into from
Jan 21, 2024

Conversation

C-Pro
Copy link
Contributor

@C-Pro C-Pro commented Nov 22, 2023

Looks like SaramaHasher is not exactly compatible with sarama.
I switched from segmentio/kafka-go to franz-go and noticed that now my keys map to different partition (this is a no-go for my usecase).

I have created a test to check key->partiton mapping consistency between sarama, segmentio and franz.
kgo.StickyKeyPartitioner(SaramaCompatHasher(fnv32a)) returns different partition number.

Test output shows franz yields different partition numbers.

So I added new SaramaCompatHasher that behaves exactly like sarama's and segmentio ones (uses int32 instead of int).
I opted to create a new instead of changing the existing SaramaHasher to maintain compatibility with old library versions.

@twmb
Copy link
Owner

twmb commented Nov 27, 2023

Good find, I'm not sure why I didn't think of the importance of the int32-ness for wrapping, considering I implemented this by having the Sarama code up at the same time.

That said, given a code search: https://github.com/search?q=kgo.SaramaHasher&type=code
The only public uses of this function are in forks, and one line in your test that found this bug.

I doubt the Sarama hasher is used by anybody. If anybody ported from Sarama to franz-go, they'd have broken hashing to begin with and would have created a bug report a long time ago. What do you think about fixing the function and not introducing a second compatibility layer?

@C-Pro
Copy link
Contributor Author

C-Pro commented Nov 27, 2023

It is ok for me, but scary still.
There can be some uses in private repos.
Even if you make it a major version bump, it can hit someone who upgraded without reading release notes.

@twmb twmb added the minor label Dec 3, 2023
pkg/kgo/partitioner.go Outdated Show resolved Hide resolved
@twmb twmb mentioned this pull request Dec 15, 2023
10 tasks
@twmb twmb merged commit 0b3766d into twmb:master Jan 21, 2024
@twmb twmb removed the minor label May 26, 2024
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.

2 participants