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

tasks and client_ids should be logical strings in the public API #1

Open
wants to merge 2 commits into
base: python2_python3_compatibility
Choose a base branch
from
Open

Conversation

lentinj
Copy link

@lentinj lentinj commented Oct 13, 2015

As in Yelp#35 there are still encoding/decoding issues in this branch, this is an attempt to fix at least the ones that stops our client and worker working.

I put the encoding of the task string at the same level as encoding the data for the job, this seemed to make sense in my head but meant I had to butcher the unit tests somewhat, as they inspect the internal job structures after sending commands. Does this seem a reasonable approach to you?

The tests pass here on Python 3.4

@svisser
Copy link
Owner

svisser commented Oct 14, 2015

Thanks for looking at this branch.

I think the main thing is the use of ASCII here. Given that we're sending bytes across and given that the Gearman protocol specification doesn't indicate what encoding should be used, it seems to me that UTF-8 could be a better choice here? This would allow us to support a broader range of identifiers. Looking into this, the OpenStack gear client also does this: https://github.com/openstack-infra/gear/blob/master/gear/__init__.py#L705.

Regarding the unit tests, I think it's fair to adapt them to make them work - we can clean / refactor them at a later date. The code has been around for a while so I'm more concerned with getting the current codebase to work in Python 3.x and letting the unit tests adequately cover the code.

@lentinj
Copy link
Author

lentinj commented Oct 14, 2015

IIRC I couldn't see anything in the gearman protocol, and copied .encode('ascii') from elsewhere in the code. I'd be happy to go utf8 instead.

Possibly the encoding should be factored out into a constant in the source somewhere? Will have a look at that.

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