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

Consumer groups on Kafka 0.9 #588

Merged
merged 4 commits into from
Jan 4, 2016
Merged

Conversation

dim
Copy link
Contributor

@dim dim commented Dec 23, 2015

Hey, I am working on implementing consumer groups on Kafka 0.9 and here are the required API additions. I am not particularly sure about metadata, as Kafka doesn't prescribe but only recommend a structure for it. I followed the recommendations on https://cwiki.apache.org/confluence/display/KAFKA/A+Guide+To+The+Kafka+Protocol#AGuideToTheKafkaProtocol-JoinGroupRequest, please let me know what you think.

Cheers,
Dim

}
Heartbeat struct {
// Interval between each heartbeat (defaults to 3s). It should be no more
// than 1/3rd of the Group.Session.Timout setting
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could check this in Config.Validate()

@wvanbergen
Copy link
Contributor

Thanks for working on this 👍

As for the approach, I think what you are doing makes sense:

  • We should follow the protocol's recommended structure for consumer groups.
  • Some of the protocol identifiers have been renamed. Due to backwards compatibility reasons, we cannot really do these renames, unless we declare a 2.0 version (something that we are considering). For now, it's probably best to just build it with the current names; the functionality is not really different.
  • Let's start with these building blocks, and tackle the higher level consumer construct in a separate PR when we land this.

@dim
Copy link
Contributor Author

dim commented Dec 23, 2015

@wvanbergen so yes, I am working on a high-level group consumer as a separate package for now. I have addressed your feedback above. Please let me know if there's anything else.

@@ -0,0 +1,94 @@
package sarama

type GroupMemberMetadata struct {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As for naming, we should probably call this ConsumerGroupMemberMetadata. This is a specialization of the generic group type.

@wvanbergen
Copy link
Contributor

This looks pretty good to me. Let's see what @eapache thinks.

@dim
Copy link
Contributor Author

dim commented Jan 3, 2016

Hey, I have released a v2 branch of https://github.com/bsm/sarama-cluster/tree/v2 which requires this PR to be merged. I would really appreciate if you could do it next week. Feedback on the cluster consumer is also welcome. Thanks, dim

@eapache
Copy link
Contributor

eapache commented Jan 3, 2016

I've been away, sorry. I'll take a look tomorrow.

eapache added a commit that referenced this pull request Jan 4, 2016
@eapache eapache merged commit 66d77e1 into IBM:master Jan 4, 2016
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.

3 participants