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

Always use OffsetFetchRequest in consumers #160

Closed
wants to merge 2 commits into from

Conversation

jersub
Copy link

@jersub jersub commented Apr 11, 2014

Hi there,

Both @mumrah and @rdiomar agree that kafka-python should only support the latest Kafka server API (cf. PR #133).

0.8.1 is the latest stable version and this library is flagged as supporting it in its README so I think it make sense to support OffsetFetchRequest out-of-box.

@wizzat
Copy link
Collaborator

wizzat commented Apr 11, 2014

I rather strongly disagree that it should only support the latest version. Especially when it's trivial to support both 0.8.0, 0.8.1, and 0.8.2. There are even multiple pull requests that do just that.

@jersub
Copy link
Author

jersub commented Apr 11, 2014

Indeed PR #142 looks fine as well. Haven't seen the last version of it when I wrote mine.

@wizzat
Copy link
Collaborator

wizzat commented Apr 11, 2014

To be fair, I think it's better to integrate your patch than to let bug report and pull requests pile up. I don't remember seeing it, but did you update the kafka-src sub module to point at 0.8.1?

On Apr 11, 2014, at 9:57, Jérémy Subtil notifications@github.com wrote:

Indeed PR #142 looks fine as well. Haven't seen the last version of it when I wrote mine.


Reply to this email directly or view it on GitHub.

@jersub
Copy link
Author

jersub commented Apr 11, 2014

To complete this PR I've just updated kafka-src as you're suggesting @wizzat. Hope this will make Travis happy.

@wizzat
Copy link
Collaborator

wizzat commented Apr 11, 2014

I think they moved to gradle in 0.8.1

@hasScott
Copy link

For clarity: Right now when using kafka-python without modifications the consumers will always start consuming from the beginning of a partition rather than from where the offset is?

As a user, and hopefully soon to be contributor, of kafka-python I would vote for enabling the consumer offset functionality even if it doesn't work with kafka 0.8.0. It is confusing that it doesn't work with Kafka 0.8.1 and worst case, to ensure backwards compatibility can't we catch the exception when a request for the offset is made against a Kafka 0.8.0 cluster?

@wizzat
Copy link
Collaborator

wizzat commented Apr 18, 2014

I don't understand why you would say kafka-python should drop support for 0.8.0 when it's still the most common kafka version AND there are multiple pull requests which address your concern and don't drop support for it.

@sr78ger
Copy link

sr78ger commented Apr 23, 2014

What about updating the readme.md

Compatible with Apache Kafka 0.8.1

with a reference to the consumer.py

https://github.com/mumrah/kafka-python/blob/master/kafka/consumer.py#L108-L116

and the necessary import of OffsetFetchRequest until this issue is fixed. It took me quite a while to figure it out.

@wizzat
Copy link
Collaborator

wizzat commented May 7, 2014

Offset management is supported as of #158. I think this can be closed now?

@dpkp
Copy link
Owner

dpkp commented May 9, 2014

Agree -- offset commits are now generated w/ autocommit. Clients connecting to older server versions should not use commit or autocommit etc.

@dpkp dpkp closed this May 9, 2014
wbarnha added a commit to orange-kao/kafka-python that referenced this pull request Mar 9, 2024
* Remove support for EOL'ed versions of Python

* Update setup.py
bradenneal1 pushed a commit to bradenneal1/kafka-python that referenced this pull request May 16, 2024
* Remove support for EOL'ed versions of Python

* Update setup.py
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.

5 participants