-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Add python 3.7 support #1614
Add python 3.7 support #1614
Conversation
27497d8
to
d96a45b
Compare
This passes locally, but seems like it's always timing out on Travis, probably due to having to use the VM builds rather than the docker container builds for python 3.7. This is what I'm hesitant to merge if we're going to constantly hit timeouts because we don't want to be fighting travis trying to make PRs go green... I'll try again tomorrow and see if I continue to hit this. |
d96a45b
to
af1c32f
Compare
See also #1549, this seems to be a valid issue but doesn't appear until 3.7. |
I feel like we have a bit too many jobs for our test runners. The PR seems good, maybe lets run it on a more recent Kafka version (>=0.11), that may (maybe) help with timeouts. |
And we need to add the 2.X Kafka brokers, so it will only get worse... I have thought several times about cutting down, but I am hesitant to do so on the broker versions as many of those are in production and they do have significant differences... maybe we should drop 3.4/3.5 from the travis matrix as most folks on python 3 tend to stay reasonably up to date
Oh, I hadn't thought of that, good idea |
In aiokafka I recently changed the matrics to only run all kafka versions with latest python and all python versions with latest kafka. Not sure if it fits for kafka-python, maybe just run all brokers for 2.7 and latest python + 1 broker per other python versions if needed. |
That sounds reasonable to me. I may not get to it soon, so feel free to do it if you get time (and no worries if you don't). Also, to put this in context... I typically look at the PR and then decide whether / which tests actually matter... ie, if it's straightforward and just needs to be tested against all brokers, then if some random python 3 version times out, I will often immediately merge. So IMO it's not too bad to have exhaustive test matrix as long as we're not too hung up on retrying builds that timed out just to make them green if we're comfortable making the judgement call that the scope of the PR doesn't create an edge case that matters for it. |
Test failure seems like there's a different java setup for this new test configuration and it is preventing Zookeeper (maybe also Kafka) from starting.
|
That's probably because of the Travis workaround: https://github.com/dpkp/kafka-python/pull/1614/files#diff-354f30a63fb0907d4ad57269548329e3R11 I'm glad it at least got that far, IIRC (it's been a little while) originally I kept seeing timeouts before even getting to that point. It'd probably be better to switch to the |
Travis xenial is now official but not the default: https://blog.travis-ci.com/2018-11-08-xenial-release That makes the workaround, less of a hack so why not try putting these lines at the top of the .travis.yml file:
That way we could at least see if KAFKA_VERSION=1.0.1 gets any further than KAFKA_VERSION=0.8.2.2. Or if that is just too much then just reverse the sort order of the env: |
af1c32f
to
e126a4e
Compare
.travis.yml
Outdated
@@ -1,10 +1,14 @@ | |||
dist: xenial # required for Python >= 3.7 (travis-ci/travis-ci#9069) | |||
sudo: required # required for Python >= 3.7 (travis-ci/travis-ci#9069) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
3925900
to
21f5f77
Compare
I think we need to continue running tests w/ Java 8 (there are some strange failures on the older kafka versions when trying to run w/ Java 11). Unfortunately I haven't figured out how to get jdk8 installed with the new travis xenial image. I opened a forum post here: https://travis-ci.community/t/how-to-use-java8-in-a-python-non-java-project-on-xenial/1823 |
Add Python 3.7 to the tests. Note that Travis requires a workaround for now. Document 3.7 support on PyPi.
…mprove tox variable handling
7f6ea22
to
0a73b18
Compare
* Use xenial dist for travis builds * Use openjdk8 for all travis tests * Update python build matrix -- add 3.7, drop 3.5/3.6 (keep 2.7, 3.4, pypy2.7)
Add Python 3.7 to the tests.
Document 3.7 support on PyPi.
This change is