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

Add redis cluster config #5799

Open
wants to merge 9 commits into
base: develop
Choose a base branch
from

Conversation

paulbauriegel
Copy link
Contributor

@paulbauriegel paulbauriegel commented Jan 31, 2025

Support Redis Cluster

We are using a redis cluster setup, but the current code uses redis.from_url which only works for Redis Standalone. See: https://github.com/redis/redis-py/blob/ceb12fe8b52f488bd1e0a42b9dc53161e77514d0/redis/utils.py#L33

So I added a config-option to support Redis Cluster which should not break the current default behavior only extend it.

Type of change

  • New feature (non-breaking change which adds functionality)
  • Documentation update

How Has This Been Tested

AWS Redis cluster deployment

Checklist

  • I added relevant documentation
  • I followed the style guidelines of this project
  • I did a self-review of my code
  • I made corresponding changes to the documentation
  • I confirm My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • TODO - I have added relevant notes to the CHANGELOG.md file (See https://keepachangelog.com/)

@paulbauriegel paulbauriegel marked this pull request as ready for review February 3, 2025 08:07
@davidberenstein1957
Copy link
Member

@paulbauriegel , would it be easy to include this too? #5800

@paulbauriegel
Copy link
Contributor Author

It's a bit of a different feature request right?
It can be added here: https://github.com/argilla-io/argilla/blob/6d6f96e498d8ffdb524e1c99aaf81af6092dc086/examples/deployments/k8s/argilla-chart/templates/deployment.yaml
by making this

value: redis://{{ .Release.Name }}-redis-master:6379/0

configurable via the values.yaml generally I can have a look but I think it's unrelated.

@paulbauriegel
Copy link
Contributor Author

It should work with two changes

  1. in the deployments.yaml
value: {{if .Values.externalRedis.enabled}}"redis://{{ .Release.Name }}-redis-master:6379/0"{{else}}"{{ .Values.externalRedis.url }}"{{end}}
  1. in the values.yml add
externalRedis:
  url: "..."

I'm just not sure if this should be part of this MR or you better set-up a new one? - Up to you @davidberenstein1957 :-)

@davidberenstein1957
Copy link
Member

Hi @paulbauriegel, adding it to this MR is fine by me :) Thanks for all the help!

Co-authored-by: David Berenstein <[email protected]>
@@ -114,6 +114,7 @@ class Settings(BaseSettings):
cors_origins: List[str] = ["*"]

redis_url: str = "redis://localhost:6379/0"
redis_use_cluster: bool = False
Copy link
Contributor Author

Choose a reason for hiding this comment

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

the idea was to have the same prefix redis_ for all settings same is for elasticsearch_ or other settings. I do not want to break with this pattern

@paulbauriegel
Copy link
Contributor Author

@davidberenstein1957 I added an externalRedis config option into the values.yml enabling both an external redis cluster or just an external redis standalone. This should fix #5800
Let me know what you think

Copy link
Member

@davidberenstein1957 davidberenstein1957 left a comment

Choose a reason for hiding this comment

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

LFTM! small remark w.r.t. changelof formatting.

@@ -114,6 +114,7 @@ class Settings(BaseSettings):
cors_origins: List[str] = ["*"]

redis_url: str = "redis://localhost:6379/0"
redis_use_cluster: bool = False
Copy link
Member

Choose a reason for hiding this comment

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

Yes, that looks clean :)

argilla-server/CHANGELOG.md Show resolved Hide resolved
Co-authored-by: David Berenstein <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants