#36020 closed Cleanup/optimization (wontfix)
redis cache backend serializing floats
Reported by: | amirreza | Owned by: | |
---|---|---|---|
Component: | Core (Cache system) | Version: | 5.1 |
Severity: | Normal | Keywords: | |
Cc: | Nick Pope | Triage Stage: | Unreviewed |
Has patch: | no | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | yes | UI/UX: | no |
Description
hi
so as you know django's redis backend https://github.com/django/django/blob/main/django/core/cache/backends/redis.py#L19-L27 doesn't serialize int values, this is to support atomic integer operation as discussed at https://github.com/django/django/pull/14437#issuecomment-915046037 and below.
but redis also has float operations, such as incrbyfloat
, and since django serializes floats these operations are not possible, at least not without some hassle
we can easily address this by not serializing floats
worth mentioning that i've recently done the same thing in django-valkey
https://github.com/amirreza8002/django-valkey/blob/main/django_valkey/base_client.py#L527-L552, and from what i can tell, it makes no difference for users and doesn't break anything.
let me know what you think
Change History (3)
comment:1 by , 20 hours ago
Cc: | added |
---|---|
Resolution: | → wontfix |
Status: | new → closed |
comment:2 by , 20 hours ago
hi
fair points
but to clarify, this is not a ticket to implement incrbyfloat
or similar methods, it's for not serializing floats, which will make float operations easier, and i feel like it's more consistent since integers are not serialized
p.s: i don't use redis backend at the moment, so i don't really insist on this, if django team wants, i'll make a PR for this, if not, we move on with life :)
comment:3 by , 20 hours ago
I understand that 🙂 this is to add a float special-case on top of the existing int special-case.
Note that folks were confused why serialization is skipped in the int
special-case for redis (see discussion in #33361).
Adding a float special-case (when I don't believe we support any float operations) feels even harder to justify.
I'm not an expert here and some folks might chime in. This can also be discussed further on the Django Forum
In investigating this ticket, I found something on
django-redis
onincrbyfloat
: https://github.com/jazzband/django-redis/issues/311#issuecomment-412816573This sounds like a request to make it easier to interact with other redis operations. I don't think that's the goal of the cache backend