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.

Sign up for GitHub

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

Add support of redis clusters #236

Merged
merged 4 commits into from
Mar 20, 2024
Merged

Add support of redis clusters #236

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
58305de
Add support of redis clusters (#1)
Millefeuille42 Mar 5, 2024
896aec0
feat: added support of additional options
Mar 18, 2024
5f25103
fix: options are passed through kwargs
Mar 18, 2024
7822729
feat: added testsuite
Mar 20, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 7 additions & 4 deletions beaker/ext/redisclusternm.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,18 +43,21 @@ def __init__(self, namespace, urls, timeout=None, **kw):
raise RuntimeError('redis is not available')

if isinstance(urls, string_type):
options = None
for url in urls.split(','):
url_options = redis.connection.parse_url(url)
if 'db' in url_options:
raise redis.cluster.RedisClusterException(
raise redis.exceptions.RedisClusterException(
"A ``db`` querystring option can only be 0 in cluster mode"
)
self.nodes.append(redis.cluster.ClusterNode(
host=url_options.get('host'),
port=url_options.get('port')
host=url_options.pop('host'),
port=url_options.pop('port')
))
if options is None:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't this odd? It seems to suggest we are only going to care about the options from the first url that has any

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems good, will add a parameter

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@Millefeuille42 Millefeuille42 Mar 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

options = url_options
self.client = RedisClusterNamespaceManager.clients.get(
urls, redis.cluster.RedisCluster, startup_nodes=self.nodes
urls, redis.cluster.RedisCluster, startup_nodes=self.nodes, **options
)
else:
self.client = urls
Expand Down