-
Notifications
You must be signed in to change notification settings - Fork 1.4k
Update consumer.py #142
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
Update consumer.py #142
Conversation
This change requires the caller to know what version the server is running. I also don't think version-aware code is the way to go because then you'll have to modify it for every future version. Also, breaks travis. |
Perhaps then a try except wrap because this does throw an exception if not available |
This problem needs some resolution, and I believe it's completely reasonable for the person making the call to know what version of Kafka the server is using. I've got a fork of Python-Kafka in production because I need to work with the full feature set of both 0.8.0 and 0.8.1. |
It is reasonable for the caller to know the version, but we need to have a solution that does not involve hard-coding version numbers in the code because it's not scalable. If we used this approach for every new server version with an api change then it just won't be practical. The correct approach is probably to release a different version of kafka-python for each api version. |
The approach that I took was to put the kafka version in the same properties file which details its location. It works pretty well. |
Even then, you still need to say
Then in the future you'll have 20 versions that support different api's, and you'll need these if/else statements in a bunch of places. |
Realistically we are only talking about differentiation between pre 0.8.1 and post 0.8.1 which is where this code becomes active James Westover
|
That's true, I'm just questioning this approach in the general sense. It's almost guaranteed that there will be more api changes in future versions, and if we want to continue to support all versions at the same time then we will have these if/else's everywhere. I'm proposing releasing a version of kafka-python that supports 0.8.0, then once 0.8.1 is released we would update kafka-python, increment our version, and release that separately. Then the user can install the appropriate package based on which server they're running, and the code stays clean. This can be done for every server upgrade that has an api change. |
That doesn't seem very scalable from the pypi perspective. The approach I took was to make the user select a class representing their particular server version. |
Other than 0.8.0-0.8.1, we will need to release a new version of kafka-python with every api change anyways, even with your approach. The difference is that we won't support older server versions and we would keep the codebase clean. If a user wants to run an older server they'll have to use an older version of kafka-python along with it. That seems reasonable to me. I'm pretty sure this is standard practice. |
This should be fixed in #158. I think we can close it? |
* Fix crc32c's __main__ for Python 3 * Remove TODO from _crc32c.py --------- Co-authored-by: Yonatan Goldschmidt <yon.goldschmidt@gmail.com>
* Fix crc32c's __main__ for Python 3 * Remove TODO from _crc32c.py --------- Co-authored-by: Yonatan Goldschmidt <yon.goldschmidt@gmail.com>
Just added a switch into the consumer so that no uncommenting is necessary for using kafka 0.8.1