Skip to content

Provide hash function choices #405

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 6 commits into from
Aug 30, 2021
Merged

Provide hash function choices #405

merged 6 commits into from
Aug 30, 2021

Conversation

cosmo0920
Copy link
Contributor

@cosmo0920 cosmo0920 commented May 26, 2021 •
edited
Loading

Fixes #397.

Kafka#new can handle Kafka::Partitioner classes which determine partitioner algorithms such as crc32 and murmur2.
In Java based kafka client, default partitioner algorithm is murmur2 not crc32.
This default algorithm glitch causes different partion assignment between Java based Kafka client and Ruby based one.
@cosmo0920 cosmo0920 requested a review from ashie May 26, 2021 08:49
@cosmo0920
Copy link
Contributor Author

@ashie @kenhys Any comments? We should provide hash function choices because default hash algorithm differs from Java based Kafka client. This causes different partition assignment for similar event patterns. What do you think?
@ashie ashie requested a review from kenhys May 27, 2021 01:31
@kenhys
Copy link
Contributor

kenhys commented May 27, 2021

I'll take a look at it.
@ashie ashie removed their request for review May 27, 2021 01:54
@ashie
Copy link
Member

ashie commented May 27, 2021

Sorry, I can't have time to review it this week.
@ashie ashie self-requested a review May 27, 2021 01:58
@ashie
Copy link
Member

ashie commented May 27, 2021

I can review it next week.
(Do you need to hurry up?)
@repeatedly
Copy link
Member

repeatedly commented May 27, 2021 •
edited
Loading

Adding note for murmur32 is better. The explanation is like below:

Java based Kafka client uses murmur32 by default. If you want to use same partitioning behavior with fluent-plugin-kafka, change it to murmur32.

Code changes are ok for me.
kenhys
kenhys reviewed May 27, 2021
View reviewed changes
@ashie ashie removed their request for review May 27, 2021 02:16
@cosmo0920
Copy link
Contributor Author

I can review it next week.
(Do you need to hurry up?)

I'm not hurry up to do this PR.
@cosmo0920 cosmo0920 force-pushed the provide-hash-function-choices branch from 6b67ec4 to 201d1e6 Compare May 27, 2021 02:32
@cosmo0920
Copy link
Contributor Author

I happened to find that murmur2 hash algorithm on ruby-kafka is still unreleased for now....
https://github.com/zendesk/ruby-kafka/blob/65e4af54131405964d62a5d07d1c52d2a3018127/CHANGELOG.md#unreleased
I'm waiting for the new stable version of ruby-kafka.
@cosmo0920 cosmo0920 force-pushed the provide-hash-function-choices branch from 201d1e6 to ae73772 Compare May 27, 2021 02:36
@cosmo0920 cosmo0920 marked this pull request as draft May 27, 2021 02:53
@cosmo0920
Copy link
Contributor Author

cosmo0920 commented May 27, 2021 •
edited
Loading

I'd turned out this PR as Draft. If ruby-kafka releases partitioner keyword argument supported version, I'll turn on this PR to be Ready for review.
@kenhys kenhys mentioned this pull request Jun 29, 2021
Closed
@github-actions
Copy link

This PR has been automatically marked as stale because it has been open 90 days with no activity. Remove stale label or comment or this PR will be closed in 30 days
@github-actions github-actions bot added the stale label Aug 25, 2021
@cosmo0920 cosmo0920 added enhancement Feature request and removed stale labels Aug 26, 2021
@cosmo0920
Copy link
Contributor Author

Hooray! ruby-kafka v1.4.0 is just released a while ago! zendesk/ruby-kafka@d5def61
ashie
ashie reviewed Aug 26, 2021
View reviewed changes
@cosmo0920
out_kafka/out_kafka2/out_kafka_buffered: Provide hash function choices
6e2e2b2 
Signed-off-by: Hiroshi Hatake <hatake@calyptia.com>
@cosmo0920
Add notes for murmur2 hash function notification
c5c7d03 
Signed-off-by: Hiroshi Hatake <hatake@calyptia.com>
@cosmo0920 cosmo0920 force-pushed the provide-hash-function-choices branch from aed87a2 to c5c7d03 Compare August 26, 2021 06:02
@cosmo0920 cosmo0920 marked this pull request as ready for review August 26, 2021 06:17
@cosmo0920
test: out_kafka: Add minimal testcase for hash_function
09f8562 
Signed-off-by: Hiroshi Hatake <hatake@calyptia.com>
@cosmo0920
test: out_kafka2: Add test cases for out_kafka2
7eb3ecf 
Signed-off-by: Hiroshi Hatake <hatake@calyptia.com>
@cosmo0920
test: Use rr for stubbing File.read behavior
75de306 
Signed-off-by: Hiroshi Hatake <hatake@calyptia.com>
@cosmo0920 cosmo0920 force-pushed the provide-hash-function-choices branch from 13de99b to 75de306 Compare August 26, 2021 09:09
@cosmo0920
test: out_kafka_buffered: Add testcases for out_kafka_buffered
f849400 
Signed-off-by: Hiroshi Hatake <hatake@calyptia.com>
@cosmo0920
Copy link
Contributor Author

I added elementally testcases for partitioner hash function.
@ashie ashie mentioned this pull request Aug 26, 2021
Closed
@cosmo0920
Copy link
Contributor Author

@kenhys @ashie Any further suggestions?
@kenhys
Copy link
Contributor

kenhys commented Aug 27, 2021

nothing from me.
@cosmo0920 cosmo0920 merged commit 211124f into master Aug 30, 2021
@cosmo0920 cosmo0920 deleted the provide-hash-function-choices branch August 30, 2021 02:06
@cosmo0920
Copy link
Contributor Author

I think that adding digest-murmurhash in fluent-agent bundled gem is enough for most users.
@cosmo0920 cosmo0920 mentioned this pull request Sep 16, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement Feature request

4 participants

@cosmo0920 @kenhys @ashie @repeatedly