Support redis-py v2 and v3#948
Conversation
|
@thedrow PTAL - this might be a better fix than my previous one. |
|
is this the only incompatible change? |
|
It seemed to be the only one that we used in this file. Unfortunately I’ve just moved house so may not have much time for a week or so to make change/fix these tests |
|
@thedrow Fixed up the stupid mistakes, and added the TODO-comment. PTAL? |
|
This looks good but the build is still failing! |
Looks like there's some kind of mock interface in the tests here: https://github.com/celery/kombu/blob/master/t/unit/transport/test_redis.py#L78 Something like this would probably work. def zadd(self, key, mapping, **kwargs):
for item in mapping
self.sets[key].add(item) |
|
When this tests were written fakeredis wasn't available or too premature :) |
|
Should I update the mock, or is it only a small amount of work to change to use fakeredis? |
|
Ah, fakeredis doesn't yet support Redis-py3 either: jamesls/fakeredis#225 |
|
Ran that test locally and it's passing now with the change Andy suggested |
Codecov Report
@@ Coverage Diff @@
## master #948 +/- ##
==========================================
+ Coverage 88.66% 88.66% +<.01%
==========================================
Files 63 63
Lines 6509 6512 +3
Branches 776 777 +1
==========================================
+ Hits 5771 5774 +3
Misses 656 656
Partials 82 82
Continue to review full report at Codecov.
|
thedrow
left a comment
thedrow
left a comment
There was a problem hiding this comment.
According to codecov, the case where redis-py 2.x is used is no longer covered.
I'd rather have a test for both cases to ensure that nothing breaks.
Further to celery#946 this fixes the underlying issue in a easy-to-upgrade way for end users, many of whom will have Redis installed via other means. By having this check here and supporting both versions concurrently it makes it easier for end users, and to use celery/kombu in projects that use Redis elsewhere. With this change it is possibly worth reverting celery#946
|
Added tests (via case.mock.replace_module_value to switch the version back and forth. I think that should bring the coverage back up. |
|
@thedrow ptal. Coverage for both versions added |
| # TODO: Remove this once we soley on Redis-py 3.0.0+ | ||
| if redis.VERSION[0] >= 3: | ||
| # Redis-py changed the format of zadd args in v3.0.0 | ||
| zadd_args = [{time(): delivery_tag}] |
There was a problem hiding this comment.
Is this not the wrong way round? The documentation says:
For ZADD, the dict is a mapping of element-names -> score.
So the key in the dict should be the element-name - in this case delivery_tag - and the value should be the score, which here is time()?
I did a quick test locally to check:
>>> from redis import Redis
>>> r = Redis()
>>> from time import time
>>> zadd_args = [{time(): 'test_tag'}]
>>> r.zadd('kombu', *zadd_args)
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
File "/Users/jeppe/.virtualenvs/tmp-82c2c5e4c227252/lib/python3.6/site-packages/redis/client.py", line 2266, in zadd
return self.execute_command('ZADD', name, *pieces, **options)
File "/Users/jeppe/.virtualenvs/tmp-82c2c5e4c227252/lib/python3.6/site-packages/redis/client.py", line 755, in execute_command
return self.parse_response(connection, command_name, **options)
File "/Users/jeppe/.virtualenvs/tmp-82c2c5e4c227252/lib/python3.6/site-packages/redis/client.py", line 768, in parse_response
response = connection.read_response()
File "/Users/jeppe/.virtualenvs/tmp-82c2c5e4c227252/lib/python3.6/site-packages/redis/connection.py", line 638, in read_response
raise response
redis.exceptions.ResponseError: value is not a valid float
>>> zadd_args2 = [{'test_tag': time()}]
>>> r.zadd('kombu', *zadd_args2)
1There was a problem hiding this comment.
I don't know - even on redis 2.10.6 I get this:
>>> import redis
>>> redis.VERSION
(2, 10, 6)
>>> from time import time
>>> r = redis.Redis()
>>> r.zadd('kombu', time(), 'test_tag')
...
redis.exceptions.ResponseError: value is not a valid floatand this is definately the same order as the code was previously.
There was a problem hiding this comment.
Oh, that's due to the difference between the Redis client and StrictRedis. In redis-py 3 StrictRedis has been renamed to Redis and StrictRedis has become an alias to Redis.
In redis-py 2 there's a difference in the input order, it's mentioned slightly in the README for the 2.10.6 release: https://github.com/andymccurdy/redis-py/tree/2.10.6#api-reference, but it might be easier to compare the code used. This is the StrictRedis.zadd method: https://github.com/andymccurdy/redis-py/blob/2.10.6/redis/client.py#L1677-L1697 and here's the Redis.zadd method: https://github.com/andymccurdy/redis-py/blob/2.10.6/redis/client.py#L2292-L2321.
>>> import redis
>>> redis.VERSION
(2, 10, 6)
>>> r = redis.StrictRedis()
>>> from time import time
>>> r.zadd('kombu', time(), 'test_tag')
1
Further to #946 this fixes the underlying issue in a easy-to-upgrade way for end users, many of whom will have Redis installed via other means. By having this check here and supporting both versions concurrently it makes it easier for end users, and to use celery/kombu in projects that use Redis elsewhere. With this change it is possibly worth reverting #946
5 participants
Further to #946 this fixes the underlying issue in a easy-to-upgrade way for end users, many of whom will have redis installed via other means. By having this check here and supporting both versions concurrently it makes it easier for end users, and to use celery/kombu in projects that use redis elsewhere.
With this change it is possibly worth reverting #946