Add support of redis clusters #236
Conversation
Add Redis cluster support as session management backend
beaker/ext/redisclusternm.py
Outdated
| )) | ||
| if options is None: |
There was a problem hiding this comment.
Isn't this odd? It seems to suggest we are only going to care about the options from the first url that has any
There was a problem hiding this comment.
This is the way I found to add options (password for example) without adding too much clutter.
Options other than port and host should be the same in every node (or at least we can specify only one with the redis cluster module).
If specifying multiple nodes, it will only parse the first one indeed.
Considering that only one set of options can be provided, what would be the best option in this case? We could merge all parsed url_options eventually.
There was a problem hiding this comment.
Adding a redis_options parameter to the RedisClusterNamespaceManager should be the best way.
Any option provided during middleware configuration gets forwarded to the cache, so like you provide the url you can provide redis_options.
If the concern is that it should be possible to provide options via .ini files too, there are options like decoding them from JSON when they are provided as a string instead of a dict.
There was a problem hiding this comment.
Seems good, will add a parameter
There was a problem hiding this comment.
About the .ini part, I'm not very fond of using another format inside a config file (like JSON inside a .ini file). What could be done is using a reference URL as redis_cluster_options, which will be parsed with redis.connection.parse_url and extract options from here.
But this seems as "weird" as using JSON inside a .ini file... Not sure of where to go from here
There was a problem hiding this comment.
Looked at how it is done in other ext/ files and adopted the kwargs solution. Options are not parsed from urls anymore but are provided through kwargs. This seems like it's how the redis-py devs intended, and it will be more sustainable than having explicit options.
beaker/ext/redisclusternm.py
Outdated
| from beaker._compat import string_type, PY2 | ||
|
|
||
|
|
||
| class RedisClusterNamespaceManager(NamespaceManager): |
There was a problem hiding this comment.
Can't this inherit from RedisNamespaceManager and only override __init__? What's the benefit of reimplementing all other methods?
Actually, is there a reason why this can't be a simple option to the RedisNamespaceManager like cluster=true? Or even if "," in urls?
There was a problem hiding this comment.
There is no point at the moment. Indeed inheriting from RedisNamespaceManager could be the best option to prevent clutter in the RedisNamespaceManager file ans to not reimplementing all.
I did not do that before because I couldn't see it elsewhere in the code and I was not sure if it was ok that some drivers depend on other's code.
Will do that
There was a problem hiding this comment.
Fixed, using inheritance and overrides to prevent reimplementing identical code.
|
Good, much better. |
|
Done ! |
|
@Millefeuille42 are you aware of any way to test redis cluster in GitHub Actions? The newly added tests for cluster are failing because the target redis node is not started in cluster mode, see https://github.com/bbangert/beaker/actions/runs/8556636486/job/23446940757#step:12:261 |
|
I think you can refer to something like this https://github.com/vishnudxb/redis-cluster which seems to provide a standard redis cluster. |
|
@Millefeuille42 nice, the testsuite correctly runs with the action that you suggested. Are you aware of a best practice to implement atomic lock release in redis cluster? See https://github.com/bbangert/beaker/blob/master/beaker/ext/redisnm.py#L125-L132 |
|
Lock on cluster has been fixed in #238 |
Add Redis cluster support as session management backend.
Fixing #235.