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
Fixed #33012 -- Added Redis cache backend. #14437
Conversation
Hey @carltongibson Do let me know what improvement I should make and how I shall proceed! |
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.
Hi @abbasidaniyal — good speedy start! 🏎
The initial thing is tests. Can you look at the coverage for the existing backends and adapt the tests for the new backend? Then, that immediately gives us a todo list in terms of API.
Sorry for stepping in. I have some questions, isn't this what jazzband/django-redis is doing? Will there be some code copying, what will be the faith of it? Should there be any discussion about it? Has it ever been one? |
Hey @WisdomPill — The proposal is to add a Redis backend to core. It will be simpler than that provided by It's been discussed quite a few times on django-developers. The swinger was last year's survey showing approx twice as many users opting for Redis (where we have no backend) over memcached (where we have several). |
Sure. I'll start working on the tests. Will be sharing coverage reports soon! |
Hey @WisdomPill! |
Hey @carltongibson!
I did not completely understand the reason for this.
Tried and tested with SQLite as well, and got the same results.
However, when I comment out this line, which call
|
Hey @abbasidaniyal — super. That's what we like to see. 😃 Can you push your latest and I will take a look hopefully tomorrow. |
I'm currently running the tests against the I'll start adapting these existing tests for the new backend now! |
0f724ec
to
9239713
Compare
Hey @carltongibson !
|
@carltongibson I was able to figure this one out. I was following the documentation to setup the DatabaseCache.
However, I believe Lines 1213 to 1220 in 225d965
Setting the "LOCATION" to some other table name leads to the test passing. Maybe we could mention it in the docs somewhere or update the test to check if duplicate table names exists? This is a little off-topic from this PR. Should I create a separate ticket for this? Or should we let it be for now? |
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.
Hi @abbasidaniyal.
Good stuff. Nice progress.
OK, so... I think time to start on the docs.
If redis-py
isn't installed, or if Redis isn't running we get a couple of errors:
django.core.cache.backends.base.InvalidCacheBackendError: Could not find backend 'django.core.cache.backends.redis.RedisCache': No module named 'redis'
and:
redis.exceptions.ConnectionError: Error 61 connecting to None:6379. Connection refused.
We should skip the tests unless Redis is set up. See the same for PyLibMCCache:
Line 1536 in 4f0a034
@unittest.skipUnless(PyLibMCCache_params, "PyLibMCCache backend not configured") |
Then updating the Running all the tests section of the contributing guide would be the next step — let's make sure that's right.
Then I think we can start adding bullet points (or better) in docs/topics/cache.txt
.
The tests are failing at two instance
- Culling
- zero and negative timeout handling : Redis-py does not support 0 or negative timeouts. I have implemented the get_backend_timeout similar to the memcache backend but I'm still not sure about how to handle 0 timeout. Ideally it should not store the key in the first place.
Noted:
3 Current failures
======================================================================
FAIL: test_cull (cache.tests.RedisCacheTests)
----------------------------------------------------------------------
Traceback (most recent call last):
File "/Users/carlton/Documents/Django-Stack/django/tests/cache/tests.py", line 650, in test_cull
self._perform_cull_test('cull', 50, 29)
File "/Users/carlton/Documents/Django-Stack/django/tests/cache/tests.py", line 647, in _perform_cull_test
self.assertEqual(count, final_count)
AssertionError: 49 != 29
======================================================================
FAIL: test_zero_cull (cache.tests.RedisCacheTests)
----------------------------------------------------------------------
Traceback (most recent call last):
File "/Users/carlton/Documents/Django-Stack/django/tests/cache/tests.py", line 653, in test_zero_cull
self._perform_cull_test('zero_cull', 50, 19)
File "/Users/carlton/Documents/Django-Stack/django/tests/cache/tests.py", line 647, in _perform_cull_test
self.assertEqual(count, final_count)
AssertionError: 49 != 19
======================================================================
FAIL: test_zero_timeout (cache.tests.RedisCacheTests)
Passing in zero into timeout results in a value that is not cached
----------------------------------------------------------------------
Traceback (most recent call last):
File "/Users/carlton/Documents/Django-Stack/django/tests/cache/tests.py", line 614, in test_zero_timeout
self.assertIsNone(cache.get('key1'))
AssertionError: 'eggs' is not None
----------------------------------------------------------------------
Let's have a think about those; need to look at them. If you can make progress on the above points meanwhile. 👍
I wouldn't worry about the 'my_cache_table'
failure — you kind of have to opt
into it — it doesn't come up unless you explicitly define a DB cache setting.
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.
I've had an initial pass through this.
I've not looking into too much detail yet at the option handling and pool/client instantiation stuff. Will look into that later.
Hey!
Now only on test fails. Handling zero timeout. Redis-py does not support it natively and django expects to no set a key with zero timeout. I'm not sure at which level should this be handled. I was wondering if we perform the check in |
@carltongibson I'm starting off with the documentation now. Should post updates in a few days! |
9239713
to
8cbb21b
Compare
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.
I've still got more to look into around the connection pooling and option handling, but this should be enough to get on with for now.
In addition, I think that you can easily implement the following in RedisCacheClient
and RedisCache
to provide better performance over the default implementations inherited from BaseCache
:
- Implement
.has_key()
usingRedis.exists()
which uses theEXISTS
command. - Implement
.incr()
usingRedis.incr()
which uses theINCRBY
command. - Implement
.decr()
usingRedis.decr()
which uses theDECRBY
command. - Implement
.delete_many()
usingRedis.delete()
which uses theDEL
command. (Note that this is the same as the normal delete, but passing multiple keys at the same time. You could changeRedisCacheClient.delete()
to take*keys
instead ofkey
, but it might need some work around the handling of.get_client()
...)
I also thought that .get_or_set()
could be enhanced to use the SET
command with NX
and GET
options. I see that using these together requires Redis >= 7.0 and GET
requires >= 6.2. Also redis-py
seems to lack support for GET
in Redis.set()
. See redis/redis-py#1412. It sounds like we may be able to optionally support this if we have a new enough version and fall back to the current version otherwise.
I hope that this helps! Once you've powered through this and got some initial documentation jotted down I'll have more of a look at the options handling and connection pool stuff.
Should I move the
Including
However, this would lead to some refactoring of code where |
Yes, we should do something like this. Although with |
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.
Could you also add redis>=3.0.0
to tests/requirements/py3.txt
?
I chose the version based on the following:
- 2.7.4 added support for
ex
,nx
, etc. to.set()
. - 2.9.1 added IPv6 support. (We support IPv6 for memcached, so makes sense.)
- 3.0.0 because November 2018 sounds better than January 2014 for 2.9.1 😆
Weirdly 2.10.6 mentions "SET's EX and PX arguments now allow values of zero." which doesn't seem to be the case. In fact, judging by redis/redis-py@3d328fa and redis/redis-py@688b400 the issue was that the error if zero was provided was not being raised.
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.
Hi @abbasidaniyal and @pope1ni — thanks for the great work thus far on this one. It's looking good already.
There's a lot of conversation: can I ask one or both of you to just comment on any points you feel outstanding from the above, so that we can put together a checklist? In the meantime I'll sit down and have a closer look, and we can agree the final TODOs. (Make sense?)
Thanks.
Hi @carltongibson. It's coming on nicely, yes. These are some of the highlights of things addressed:
There are some outstanding comments that still need to be addressed:
Other things that need attention:
Once this is done, I think that it would be good to land this as a very simple backend that essentially mirrors what the memcached backends can do. So as long as we have one or multiple servers and that they can support UNIX sockets, IPv6, possibly TLS -- then we have a drop-in replacement. That would be the first phase. Adding sharding, etc. could then be considered in a follow up PR. As long as we have a very simple implementation here and now, extending for other things should be easy. Some other thoughts:
Thanks @abbasidaniyal for all your efforts so far. This is coming along nicely. |
7b39478
to
cc06f52
Compare
Hey everyone! However, there is still work left and I hope to finish it as soon as possible! I've made few updates to the PR. Let me know if there are any feedbacks! Since this comment/review section has become huge, I thought of maintaining a |
@abbasidaniyal Thanks 👍 I moved it to a separate PR, #14827. |
271ad20
to
17cf539
Compare
Rebased. |
17cf539
to
0e454e5
Compare
@abbasidaniyal I rebased after 301a85a and 42dfa97. As far as I'm aware we have two pending comments: #14437 (comment), #14437 (comment) (\cc @carltongibson), and optimized implementation of |
Hey!
So since we are serializing the data before storing it into redis, we can not use the Waiting on more inputs on the cache argument comment! |
Thanks for the clarification. |
I understand this, but I'm wondering how this is working for the memcached backends for which we're also pickling the value. Looking at Looking into the default client it looks as though https://github.com/jazzband/django-redis/blob/827eb4e2607189fab8f6e22c67be1068c45ca72f/django_redis/client/default.py#L552-L571 But I think our issue is that we're pickling all values irrespective of type. Perhaps we should do the same as So in that case we'd be storing integers as integers which is probably more efficient and would allow these Sorry I didn't really pick up on this earlier. |
@ngnpope I tried changing What do you think? |
OK, It's probably worth because we will have "better atomicity" ( |
0e454e5
to
57b9f80
Compare
307e63e
to
5abe640
Compare
@abbasidaniyal @felixxm I've pushed docs edits addressing those points. Thanks! |
8700288
to
4a60dd7
Compare
Thanks Carlton Gibson, Chris Jerdonek, David Smith, Keryn Knight, Mariusz Felisiak, and Nick Pope for reviews and mentoring this Google Summer of Code 2021 project.
4a60dd7
to
ec212c6
Compare
Great work @abbasidaniyal! Thanks for your work over GSoC! 🎁 |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Thank you @carltongibson and everyone who helped me during this past 3 months! |
ticket-33012
This PR is in accordance with this GSoC project
The detailed proposal can be found here
This PR aims at adding support for Redis to be used as a caching backend with Django. As redis is the most popular caching backend, adding it to django.core.cache module would be a great addition for developers who previously had to rely on the use of third party packages.
Major Tasks :
PickleSerializer
fromdjango.contrib.sessions.serializers
todjango.core.serializers.base
. See comment Refs #33012 -- Moved PickleSerializer to django.core.serializers.base and added tests. #14827Remaining Tasks :
Future Tasks :
username
andpassword
inOPTIONS
. This will be possible in the upcoming version of redis-py