Skip to content
This repository has been archived by the owner on Apr 3, 2019. It is now read-only.

Removed unicode encoding for strings that might not be unicode #50

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

njo
Copy link

@njo njo commented Nov 23, 2013

Reverted where the code was attempting to unicode encode a value pulled from redis. The to_unicode would cause an exception to be thrown if there was a multibulk reply (eg. HMGET myhash field1 field2) and the values stored in redis were not unicode data - ie. gzip data.

@leporo
Copy link
Owner

leporo commented Nov 23, 2013

Please check out the unit-test output: https://travis-ci.org/leporo/tornado-redis/builds/14397546
I can't merge your change at the moment.

BTW, tornado-redis' performance with MGET/MSET commands is really poor. May I suggest you to try using redis-py (synchronous client) in your application for MGET/MSET commands? Please benchmark your code to be sure it performs better. Feel free to ask me if you need any assistance.

See the issue #36 for details on poor MGET/MSET performance.

I think we may get far better performance by using the hiredis parser for reply processing. There are just too many callbacks in multi-bulk reply handling.

@gmr
Copy link

gmr commented Mar 6, 2014

The unicode bit is biting me with binary data as well. Any chance we can see this addressed? @leporo is the main issue the tests needing updates?

@leporo
Copy link
Owner

leporo commented Mar 6, 2014

The main issue with the proposed fix is that it may ruin existing applications.

I'll review client's code when I have spare time.

@rudolphfroger
Copy link

Please fix this, putting binary data into Redis is quite common I believe (like struct packed integers, hash digests etc). Contact me if there's anything I can do to help with this.

@zjjott
Copy link

zjjott commented Sep 13, 2016

+1

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants