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

Fixed #33012 -- Added Redis cache backend. #14437

Merged
merged 1 commit into from Sep 14, 2021

Conversation

abbasidaniyal
Copy link
Contributor

@abbasidaniyal abbasidaniyal commented May 24, 2021

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 :

Remaining Tasks :

Future Tasks :

  • Add support for providing username and password in OPTIONS. This will be possible in the upcoming version of redis-py
  • Milli second support (To be discussed)

@abbasidaniyal
Copy link
Contributor Author

Hey @carltongibson
So I was able to get a quick and simple implementation up and running. However, there are several decisions that I made for simplicity, like using pickle for serializing data.
I am still not sure how shall be handle multiple servers. Do we setup a sharding based client (like memcached) or should it cater to a replication based setup with a "Primary and replica" based setup?

Do let me know what improvement I should make and how I shall proceed!

@carltongibson carltongibson self-requested a review May 25, 2021 12:44
Copy link
Member

@carltongibson carltongibson left a 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.

@WisdomPill
Copy link

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?
By the way django-redis is already somewhat compatible with django api and widely used by the community.

Should there be any discussion about it? Has it ever been one?

@carltongibson
Copy link
Member

Hey @WisdomPill — The proposal is to add a Redis backend to core.

It will be simpler than that provided by django-redis, for instance customising the serialiser is out-of-scope for the initial pass.

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).

@abbasidaniyal
Copy link
Contributor Author

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.

Sure. I'll start working on the tests. Will be sharing coverage reports soon!

@abbasidaniyal
Copy link
Contributor Author

abbasidaniyal commented May 26, 2021

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?
By the way django-redis is already somewhat compatible with django api and widely used by the community.

Should there be any discussion about it? Has it ever been one?

Hey @WisdomPill!
You can find a link to my proposal here. A lot of inspiration is taken from the django-redis package as mentioned in the proposal. There might be some code similarities with django-redis as the end-goal of remains the same, that is, connecting redis-py client with the django caching framework.

@abbasidaniyal
Copy link
Contributor Author

abbasidaniyal commented May 31, 2021

Hey @carltongibson!
So I was going trying to work on the coverages of each backend. However, one tests keeps failing.
cache.tests.CreateCacheTableForDBCacheTests.test_createcachetable_observes_database_router

======================================================================
FAIL: test_createcachetable_observes_database_router (cache.tests.CreateCacheTableForDBCacheTests)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/daniyal/Desktop/django/django/test/utils.py", line 430, in inner
    return func(*args, **kwargs)
  File "/home/daniyal/Desktop/django/tests/cache/tests.py", line 1238, in test_createcachetable_observes_database_router
    management.call_command('createcachetable', database='other', verbosity=0)
  File "/home/daniyal/Desktop/django/django/test/testcases.py", line 86, in __exit__
    self.test_case.assertEqual(
AssertionError: 1 != 5 : 1 queries executed, 5 expected
Captured queries were:
1. 
            SELECT c.relname,
            CASE WHEN c.relispartition THEN 'p' WHEN c.relkind IN ('m', 'v') THEN 'v' ELSE 't' END
            FROM pg_catalog.pg_class c
            LEFT JOIN pg_catalog.pg_namespace n ON n.oid = c.relnamespace
            WHERE c.relkind IN ('f', 'm', 'p', 'r', 'v')
                AND n.nspname NOT IN ('pg_catalog', 'pg_toast')
                AND pg_catalog.pg_table_is_visible(c.oid)
        

----------------------------------------------------------------------

I did not completely understand the reason for this.
My test settings were

DATABASES = {
    'default': {
        'ENGINE': 'django.db.backends.postgresql',
        'NAME': 'mydb_default',
        'USER': 'myuser',
        'PASSWORD': 'password',
        'HOST': 'localhost',
        'PORT': '5432',
    },
    'other': {
        'ENGINE': 'django.db.backends.postgresql',
        'NAME': 'mydb_other',
        'USER': 'myuser',
        'PASSWORD': 'password',
        'HOST': 'localhost',
        'PORT': '5432',
    }
}

CACHES = {
    "default": {
        "BACKEND": "django.core.cache.backends.db.DatabaseCache",
        "LOCATION": "my_cache_table",
    },
}

Tried and tested with SQLite as well, and got the same results.
Error with SQLite

======================================================================
FAIL: test_createcachetable_observes_database_router (cache.tests.CreateCacheTableForDBCacheTests)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/daniyal/Desktop/django/django/test/utils.py", line 430, in inner
    return func(*args, **kwargs)
  File "/home/daniyal/Desktop/django/tests/cache/tests.py", line 1238, in test_createcachetable_observes_database_router
    management.call_command('createcachetable', database='other', verbosity=0)
  File "/home/daniyal/Desktop/django/django/test/testcases.py", line 86, in __exit__
    self.test_case.assertEqual(
AssertionError: 1 != 5 : 1 queries executed, 5 expected
Captured queries were:
1. 
            SELECT name, type FROM sqlite_master
            WHERE type in ('table', 'view') AND NOT name='sqlite_sequence'
            ORDER BY name

----------------------------------------------------------------------

However, when I comment out this line, which call createcachetable, the test passes.

call_command('createcachetable', database=self.connection.alias)

@carltongibson
Copy link
Member

Hey @abbasidaniyal — super. That's what we like to see. 😃

Can you push your latest and I will take a look hopefully tomorrow.

@abbasidaniyal
Copy link
Contributor Author

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 main branch.
Sharing the coverage results as screenshots here

I'll start adapting these existing tests for the new backend now!

@abbasidaniyal
Copy link
Contributor Author

Hey @carltongibson !
I've just pushed the lastest update that I have. I've adapted the existsing tests for the new backend. 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.

@abbasidaniyal
Copy link
Contributor Author

Hey @carltongibson!
So I was going trying to work on the coverages of each backend. However, one tests keeps failing.
cache.tests.CreateCacheTableForDBCacheTests.test_createcachetable_observes_database_router

======================================================================
FAIL: test_createcachetable_observes_database_router (cache.tests.CreateCacheTableForDBCacheTests)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/daniyal/Desktop/django/django/test/utils.py", line 430, in inner
    return func(*args, **kwargs)
  File "/home/daniyal/Desktop/django/tests/cache/tests.py", line 1238, in test_createcachetable_observes_database_router
    management.call_command('createcachetable', database='other', verbosity=0)
  File "/home/daniyal/Desktop/django/django/test/testcases.py", line 86, in __exit__
    self.test_case.assertEqual(
AssertionError: 1 != 5 : 1 queries executed, 5 expected
Captured queries were:
1. 
            SELECT c.relname,
            CASE WHEN c.relispartition THEN 'p' WHEN c.relkind IN ('m', 'v') THEN 'v' ELSE 't' END
            FROM pg_catalog.pg_class c
            LEFT JOIN pg_catalog.pg_namespace n ON n.oid = c.relnamespace
            WHERE c.relkind IN ('f', 'm', 'p', 'r', 'v')
                AND n.nspname NOT IN ('pg_catalog', 'pg_toast')
                AND pg_catalog.pg_table_is_visible(c.oid)
        

----------------------------------------------------------------------

I did not completely understand the reason for this.
My test settings were

DATABASES = {
    'default': {
        'ENGINE': 'django.db.backends.postgresql',
        'NAME': 'mydb_default',
        'USER': 'myuser',
        'PASSWORD': 'password',
        'HOST': 'localhost',
        'PORT': '5432',
    },
    'other': {
        'ENGINE': 'django.db.backends.postgresql',
        'NAME': 'mydb_other',
        'USER': 'myuser',
        'PASSWORD': 'password',
        'HOST': 'localhost',
        'PORT': '5432',
    }
}

CACHES = {
    "default": {
        "BACKEND": "django.core.cache.backends.db.DatabaseCache",
        "LOCATION": "my_cache_table",
    },
}

Tried and tested with SQLite as well, and got the same results.
Error with SQLite

======================================================================
FAIL: test_createcachetable_observes_database_router (cache.tests.CreateCacheTableForDBCacheTests)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/daniyal/Desktop/django/django/test/utils.py", line 430, in inner
    return func(*args, **kwargs)
  File "/home/daniyal/Desktop/django/tests/cache/tests.py", line 1238, in test_createcachetable_observes_database_router
    management.call_command('createcachetable', database='other', verbosity=0)
  File "/home/daniyal/Desktop/django/django/test/testcases.py", line 86, in __exit__
    self.test_case.assertEqual(
AssertionError: 1 != 5 : 1 queries executed, 5 expected
Captured queries were:
1. 
            SELECT name, type FROM sqlite_master
            WHERE type in ('table', 'view') AND NOT name='sqlite_sequence'
            ORDER BY name

----------------------------------------------------------------------

However, when I comment out this line, which call createcachetable, the test passes.

call_command('createcachetable', database=self.connection.alias)

@carltongibson I was able to figure this one out. I was following the documentation to setup the DatabaseCache.

CACHES = {
    "default": {
        "BACKEND": "django.core.cache.backends.db.DatabaseCache",
        "LOCATION": "my_cache_table",
    },
}

However, I believe my_cache_table was conflicting with this

django/tests/cache/tests.py

Lines 1213 to 1220 in 225d965

@override_settings(
CACHES={
'default': {
'BACKEND': 'django.core.cache.backends.db.DatabaseCache',
'LOCATION': 'my_cache_table',
},
},
)

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?

Copy link
Member

@carltongibson carltongibson left a 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:

@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.

#14437 (comment):

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.

Copy link
Member

@ngnpope ngnpope left a 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.

django/core/cache/backends/redis.py Outdated Show resolved Hide resolved
django/core/cache/backends/redis.py Outdated Show resolved Hide resolved
django/core/cache/backends/redis.py Outdated Show resolved Hide resolved
django/core/cache/backends/redis.py Outdated Show resolved Hide resolved
django/core/cache/backends/redis.py Outdated Show resolved Hide resolved
tests/cache/tests.py Outdated Show resolved Hide resolved
tests/cache/tests.py Outdated Show resolved Hide resolved
django/core/cache/backends/redis.py Outdated Show resolved Hide resolved
django/core/cache/backends/redis.py Outdated Show resolved Hide resolved
@abbasidaniyal
Copy link
Contributor Author

Hey!
I've made the changes suggested by @pope1ni. I've updated the tests and added

redis_excluded_caches = {'cull', 'zero_cull'}
...
@override_settings(CACHES=caches_setting_for_tests(
    base=RedisCache_params,
    exclude=redis_excluded_caches
))
class RedisCacheTests(BaseCacheTests, TestCase):
    ...

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 get_backend_timeout method and return a value (eg: None) or raise a suitable exception.

@abbasidaniyal
Copy link
Contributor Author

@carltongibson I'm starting off with the documentation now. Should post updates in a few days!

Copy link
Member

@ngnpope ngnpope left a 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:

  1. Implement .has_key() using Redis.exists() which uses the EXISTS command.
  2. Implement .incr() using Redis.incr() which uses the INCRBY command.
  3. Implement .decr() using Redis.decr() which uses the DECRBY command.
  4. Implement .delete_many() using Redis.delete() which uses the DEL command. (Note that this is the same as the normal delete, but passing multiple keys at the same time. You could change RedisCacheClient.delete() to take *keys instead of key, 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.

django/core/cache/backends/redis.py Outdated Show resolved Hide resolved
django/core/cache/backends/redis.py Outdated Show resolved Hide resolved
django/core/cache/backends/redis.py Outdated Show resolved Hide resolved
django/core/cache/backends/redis.py Outdated Show resolved Hide resolved
django/core/cache/backends/redis.py Outdated Show resolved Hide resolved
django/core/cache/backends/redis.py Outdated Show resolved Hide resolved
django/core/cache/backends/redis.py Outdated Show resolved Hide resolved
django/core/cache/backends/redis.py Outdated Show resolved Hide resolved
django/core/cache/backends/redis.py Outdated Show resolved Hide resolved
django/core/cache/backends/redis.py Outdated Show resolved Hide resolved
django/core/cache/backends/redis.py Outdated Show resolved Hide resolved
@abbasidaniyal
Copy link
Contributor Author

abbasidaniyal commented Jun 25, 2021

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'

Should I move the import redis line inside the __init__ method of the RedisCache class? All memcache backend do that and the error raised when a binding is not installed is like this

ModuleNotFoundError: No module named 'pymemcache'

Including import redis at the top leads to an error message as you mentioned above.
I wanted to move the import command like this

class RedisCache(BaseCache):
    def __init__(self, server, params):
        import redis
        super().__init__(params)
        if isinstance(server, str):
            self._servers = re.split('[;,]', server)
        else:
            self._servers = server

        self._class = RedisCacheClient
        self._options = params.get('OPTIONS') or {}

However, this would lead to some refactoring of code where redis.ConnectionPool etc are used.
What do you think about this?

@ngnpope
Copy link
Member

ngnpope commented Jun 25, 2021

Should I move the import redis line inside the __init__ method of the RedisCache class? All memcache backend do that and the error raised when a binding is not installed is like this

Yes, we should do something like this. Although with pymemcache it is done in PyMemcacheCache.__init__() because pymemcache provides the client class. We're writing the client class ourselves, so we want import redis in RedisCacheClient.__init__().

Copy link
Member

@ngnpope ngnpope left a 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.

django/core/cache/backends/redis.py Outdated Show resolved Hide resolved
django/core/cache/backends/redis.py Outdated Show resolved Hide resolved
django/core/cache/backends/redis.py Outdated Show resolved Hide resolved
django/core/cache/backends/redis.py Outdated Show resolved Hide resolved
django/core/cache/backends/redis.py Outdated Show resolved Hide resolved
django/core/cache/backends/redis.py Outdated Show resolved Hide resolved
django/core/cache/backends/redis.py Outdated Show resolved Hide resolved
django/core/cache/backends/redis.py Outdated Show resolved Hide resolved
docs/topics/cache.txt Outdated Show resolved Hide resolved
docs/topics/cache.txt Outdated Show resolved Hide resolved
Copy link
Member

@carltongibson carltongibson left a 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.

tests/cache/tests.py Outdated Show resolved Hide resolved
@ngnpope
Copy link
Member

ngnpope commented Jul 14, 2021

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:

  • As mentioned in Fixed #33012 -- Added Redis cache backend. #14437 (review) some operations still need to be implemented:
    • Implement atomic/optimized .incr() using Redis.incr() which uses the INCRBY command.
    • Implement atomic/optimized .decr() using Redis.decr() which uses the DECRBY command.
  • As Redis supports timeouts in milliseconds, do we want to allow that? (What are your thoughts @carltongibson?)
    • It would be nice as it has often come up as a complaint that memcached doesn't support this.
    • We would still keep timeout in floating point seconds for compatibility, but scale up in .get_backend_timeout().
    • Then we would switch over from ex to px and .expire() to .pexpire(), etc.
  • Redis has 16 logical databases, so, although most people will use the default, we should allow configuration of that.

Other things that need attention:

  • Documentation. I've only made brief comments, but it should be looked at thoroughly.
  • Release notes. None have been added yet. This will be a headline feature, obviously.
  • Tests. Currently we're piggybacking off the cross-backend tests nicely. But are there any gaps or other specific things to test?
  • How the connection pooling, etc. works. I haven't reviewed this at all yet.
  • What OPTIONS do we explicitly want to handle before being passed on to the underlying library?
    • We'll need to forward **kwargs at the very least so that users have the flexibility.
    • Is there anything we want to set by default? Check out redis.Redis().

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:

  • We don't need to consider culling. That is only necessary for services that do not handle expiry themselves, e.g. databases.
    • Which is why it is not implemented for the memcached backends.

Thanks @abbasidaniyal for all your efforts so far. This is coming along nicely.

@abbasidaniyal
Copy link
Contributor Author

Hey everyone!
First of all, I'd like to thank each and everyone who has helped me on this project. GSoC ended last week for me and it has been a wonderful experience. I've learned a lot during this time and I'm greatful to each and everyone who helped me during the program! 😄🥳

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 Remaining Tasks section in the description. Also, if I missed out on any comments/suggestion above, please do let me know!

@felixxm
Copy link
Member

felixxm commented Sep 2, 2021

@abbasidaniyal Thanks 👍 I moved it to a separate PR, #14827.

@felixxm
Copy link
Member

felixxm commented Sep 2, 2021

Rebased.

@felixxm
Copy link
Member

felixxm commented Sep 8, 2021

@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 incr(), decr() is missing as mentioned in #14437 (review) (\cc @ngnpope).

@abbasidaniyal
Copy link
Contributor Author

Hey!

As far as I'm aware we have two pending comments: #14437 (comment), #14437 (comment) (\cc @carltongibson), and optimized implementation of incr(), decr() is missing as mentioned in #14437 (review) (\cc @ngnpope).

So since we are serializing the data before storing it into redis, we can not use the INCR / DECR commands that redis provides.

Waiting on more inputs on the cache argument comment!

@felixxm
Copy link
Member

felixxm commented Sep 8, 2021

So since we are serializing the data before storing it into redis, we can not use the INCR / DECR commands that redis provides.

Thanks for the clarification.

@ngnpope
Copy link
Member

ngnpope commented Sep 8, 2021

So since we are serializing the data before storing it into redis, we can not use the INCR / DECR commands that redis provides.

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 django-redis, it is defining incr() -- for simplicity I'll skip doubling this up for decr() -- in the cache backend:

https://github.com/jazzband/django-redis/blob/827eb4e2607189fab8f6e22c67be1068c45ca72f/django_redis/cache.py#L128-L129

Looking into the default client it looks as though django-redis attempts to brute force an increment and falls back to .get() and .set():

https://github.com/jazzband/django-redis/blob/827eb4e2607189fab8f6e22c67be1068c45ca72f/django_redis/client/default.py#L552-L571
https://github.com/jazzband/django-redis/blob/827eb4e2607189fab8f6e22c67be1068c45ca72f/django_redis/client/default.py#L500-L550

But .get() and .set() isn't particularly great as it loses the atomicity of .incr()/.decr().

I think our issue is that we're pickling all values irrespective of type. Perhaps we should do the same as pymemcache (which is presumably the same as the other memcached clients given that the tests pass) and selectively pickle:

https://github.com/pinterest/pymemcache/blob/0ecdc4c85f71c1a40a8f43e6251bcf68b6e911d2/pymemcache/serde.py#L36-L60

So in that case we'd be storing integers as integers which is probably more efficient and would allow these .incr()/.decr() operations to work.

Sorry I didn't really pick up on this earlier.

@felixxm
Copy link
Member

felixxm commented Sep 13, 2021

@ngnpope I tried changing incr() and decr(). Unfortunately, Redis.incr()/decr() don't raise an error when the value doesn't exists, so we'd have to add call exists() anyway. I'm not sure if it's worth it 🤔

What do you think?

@felixxm
Copy link
Member

felixxm commented Sep 13, 2021

I'm not sure if it's worth it 🤔

OK, It's probably worth because we will have "better atomicity" (incr() instead of get()/set()).

@carltongibson
Copy link
Member

As far as I'm aware we have two pending comments: #14437 (comment), #14437 (comment) (\cc @carltongibson)

@abbasidaniyal @felixxm I've pushed docs edits addressing those points. Thanks!

@felixxm felixxm removed the request for review from ngnpope September 14, 2021 10:08
@felixxm felixxm force-pushed the redis-cache-backend branch 2 times, most recently from 8700288 to 4a60dd7 Compare September 14, 2021 11:39
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.
@felixxm felixxm merged commit ec212c6 into django:main Sep 14, 2021
@carltongibson
Copy link
Member

Great work @abbasidaniyal! Thanks for your work over GSoC! 🎁

@chadbrewbaker

This comment has been minimized.

@adamchainz

This comment has been minimized.

@abbasidaniyal
Copy link
Contributor Author

Great work @abbasidaniyal! Thanks for your work over GSoC!

Thank you @carltongibson and everyone who helped me during this past 3 months!

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