Skip to content

Allow the coordinator to auto-commit for api_version==0.8.1 #1832

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

Merged
merged 1 commit into from
Jun 20, 2019

Conversation

justecorruptio
Copy link
Contributor

@justecorruptio justecorruptio commented Jun 3, 2019

Raised once in #1594

We're using api_version=(0, 8, 1) and enable_auto_commit=True. This had worked in kafka-python==1.3.5 but fails now in kafka-python==1.4.6.

In 1.3.5 even though the documentation suggested that commits to ZooKeeper is not supported, we could see the commit reflected upon inspection with zkCli.

In 1.4.6 though, KafkaConsumer just doesn't function with the above configuration; because the early exit in ConsumerCoordinator.poll() prevents the coordinator from updating next_auto_commit_deadline. This stale value, in turn, causes KafkaConsumer._message_generator() to preempt the iteration of self._fetcher until consumer_timeout is reached.

If we allow for 0.8.1 in ConsumerCoordinator.poll(), then everything seems fine. Messages are yielded, and commits are written to ZK.


This change is Reviewable

Copy link
Contributor

@jeffwidman jeffwidman left a comment

Choose a reason for hiding this comment

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

This seems safe to me, although it's weird that zookeeper commits work at all for KafkaConsumer!

0.8.2 is required for supporting commits to kafka's __consumer_offsets topic, so that's probably why this requirement is in there.

You didn't see any other unexpected issues when lowering this requirement? I was half-expecting something else to blow up if it was also trying to commit to kafka...

@jeffwidman
Copy link
Contributor

Also, I don't remember this far back... in 0.8.1, was the client issuing the commits to zookeeper, or was it still issuing the commits to the broker and then the broker piped them through to zookeeper? If the latter, that would explain why this works... and in fact we may want to just remove the requirement altogether (at least at this point in the code)

@justecorruptio
Copy link
Contributor Author

Mmm, I'm not familiar with anything past 0.8.1 actually. But I haven't seen any issues, and 1.3.5 was working with this setup.

Yes, 0.8.1 is where the writes to zookeeper are from the broker instead of the client.

@jeffwidman
Copy link
Contributor

Yes, 0.8.1 is where the writes to zookeeper are from the broker instead of the client.

If that's the case, I'd prefer to simply change this to if self.group_id is None: and drop the version check altogether.

If you do that, I'll happily merge... this is very low risk as it won't change behavior for anyone > 0.8.1, and that's the vast majority of our users.

@justecorruptio
Copy link
Contributor Author

aye aye!

@jeffwidman jeffwidman merged commit 5e055bc into dpkp:master Jun 20, 2019
@jeffwidman
Copy link
Contributor

Thank you again!

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