Opened 24 hours ago

Closed 20 hours ago

Last modified 20 hours ago

#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 Sarah Boyce, 20 hours ago

Cc: Nick Pope added
Resolution: wontfix
Status: newclosed

In investigating this ticket, I found something on django-redis on incrbyfloat: https://github.com/jazzband/django-redis/issues/311#issuecomment-412816573

[This] is not a redis client, [this] is a django cache backend. It should implement the interface of django cache backend. If you want access to redis funcionality you can access to redis connection directly using http://niwinz.github.io/django-redis/latest/#_raw_client_access that already exposes all redis functionality and we dont need reimplement it.

This 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

comment:2 by amirreza, 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 Sarah Boyce, 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

Note: See TracTickets for help on using tickets.
Back to Top