Fixing copy() methods to work with gevent library

[hmahmood]

This fixes an exception where calling copy() on a KafkaClient object after monkey patching with gevent. So code like this:

import gevent.monkey; gevent.monkey.patch_all()
import kafka

c=kafka.KafkaClient(['localhost:9092'])
c.copy()

[blafountain]

👍 i ran into this issue and needed to fork + apply

[wizzat]

Hey I'm having some trouble making this practically work. Could you take a look at this branch and let me know how to modify the test suite to work? https://github.com/wizzat/kafka-python/tree/gevent_support

[hmahmood]

Did you get the patch I sent to you a few days ago?

[wizzat]

No; I will look for it today. That branch has been merged into mumrah/master so maybe the best place to put it is in your current PR.

-Mark

On May 8, 2014, at 9:00, hmahmood [email protected] wrote:

Did you get the patch I sent to you a few days ago?

—
Reply to this email directly or view it on GitHub.

[wizzat]

Actually, that branch needs mumrah/master merged into it. I forgot that I'd created a special branch for this. Still, I can't find the patch. Can you create a PR against that branch and I'll work on merging it?

[hmahmood]

I am having a hard time getting the test suite to pass with the gevent tests in there. Without the gevent monkey patch, tests pass fine with this change. Can we just pull this change in for now, and just note that gevent support is untested? I have had the kafka client running with gevent monkey patch at work just fine for months now, so I have little doubts about whether it works. I think the test suite is just not structured in a way that will cleanly let us test gevent support.

[wizzat]

The purpose of the test suite is to verify that things continue to work as expected. It's entirely reasonable that the patch only works for the specific way that you're using gevent+kafka in production. I'd be infinitely more comfortable if we could make the test suite pass with the gevent changes. Do you have some suggestions for how to restructure the test suite to allow gevent support to be tested cleanly?

[hmahmood]

It does pass with the code changes; the changes don't reference gevent at all. The problem is using gevent in the test suite itself. I suspect it has something to do with using multiple processes in the tests, but I'd venture a guess that changes to the test suite would be non-trivial to get working with gevent. As it stands, the code changes don't break existing functionality, at least according to the test suite. If these changes get pulled in, all it means is that gevent support is experimental and untested.

[wizzat]

Yes, of course. That's not what I'm getting at. What I'm trying to get at is that I'd like to either have formal gevent support (with a test suite to back that up) or - at the very least - not play whackamole with changes that break kafka+gevent. I'm dubious to the idea that the subprocess calls are what break gevent support, because most of the errors I saw trying to integrate the changes were related to timeouts not being respected. That leads me to the conclusion that kafka-python may not actually work with gevent even with your changes - though I have no doubt that it works for your specific use case.

[hmahmood]

The only tests that are failing after my changes (I haven't pushed all of them to my branch yet) are the tests that use the multiprocessing module either via SimpleProducer or MultiprocessConsumer. Mixing multiprocessing with gevent has known issues, since forking after patching with gevent leaves gevent in a bad state. I can add a decorator to skip these tests when we test with gevent. Is that acceptable?

[wizzat]

I think its eminently reasonable to skip async tests when USE_GEVENT is set.

[hmahmood]

Please take a look at the way the tests are set up. I could not figure out a way to cleanly update the travis build settings.

[wizzat]

Yep, looking at it. It looks like you accidentally deleted the servers/0.8.0 subproject in one of your commits.

[hmahmood]

Yes the merge from master didn't got well with that submodule. I could not figure out how to fix this though.

[wizzat]

I'm going to try a fresh checkout of master and a merge --squash from your branch

[hmahmood]

I got rid of test_gevent.py and went with using an env var for patching with gevent. I think this is a better approach since adding a new test file won't mean adding it to test_gevent.py as well. I disabled running tests under pypy with gevent, since gevent does not seem to work with pypy.

[hmahmood]

Mark, were you able to get the merge going?

[wizzat]

No, work dropped a nuke on my lap 10 minutes after I started the merge and I haven't managed to get back to it. I'll make a pass at it today after I finish a few really important tasks at work.

[wizzat]

https://github.com/wizzat/kafka-python/tree/gevent_merge

[hmahmood]

This seems to have "squashed" the client.py and conn.py files where they neither have my changes nor do they match up with master.

[wizzat]

I don't see anything wrong with the merge. Your changes are in it, and it matches very closely with your branch. The reason that conn.py doesn't match your branch is because your branch isn't up to date with changes from master (in particular an IPv6 commit from earlier this week). Client.py is literally your file. I created the branch with the following process:

 git checkout fix-copy
 git pull
 git checkout master
 git pull
 git checkout -b gevent_merge
 git merge fix-copy --squash
 vim setup.py # Fix conflict
 git add setup.py
 git diff --cached > patch
 git reset HEAD --hard
 git apply patch
 git commit -m 'Merge commit'

[hmahmood]

Sorry ... was looking at the wrong branch. Things look good except for the travis_run_tests.sh scripts. I have a couple of fixes for that and tox.ini. How do I get those to you?

[wizzat]

I think the best way to handle it would be to fork my branch and make a new PR to Kafka-Python from it.

[hmahmood]

That would meaning withdrawing this pull request, deleting my fork of kafka-python and the working off of a new fork of your fork of kafka-python?

[hmahmood]

Can you merge your branch into master and then I can do another pull request to fix the travis build?

[wizzat]

What?

[dpkp]

replaced by #172