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

feat: add redis cluster support and redis_client is not none #4520

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

maldinixiang
Copy link

@maldinixiang maldinixiang commented May 20, 2024

Description

This change add redis cluster support . redis-py should use latest version which has redis.cluster
First, add variable in config to set redis cluster if enabled
Second, add variable to environment so that some class in util such as model_provider_cache.py can get a redis_client instance which not none, do not add to environment and init redis_client in init_app(app) with got such error
File "/home/service/app/dify/api/core/helper/model_provider_cache.py", line 24, in get cached_provider_credentials = redis_client.get(self.cache_key) AttributeError: 'NoneType' object has no attribute 'get'

Fixes # (issue)

Type of Change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)

How Has This Been Tested?

Set REDIS_CLUSTER_ENABLED in config and create admin account success

Suggested Checklist:

  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • My changes generate no new warnings
  • I ran dev/reformat(backend) and cd web && npx lint-staged(frontend) to appease the lint gods

@dosubot dosubot bot added size:M This PR changes 30-99 lines, ignoring generated files. 💪 enhancement New feature or request labels May 20, 2024
Comment on lines +81 to +85
os.environ['REDIS_CLUSTER_ENABLED'] = app.config['REDIS_CLUSTER_ENABLED']
os.environ['REDIS_USERNAME'] = app.config['REDIS_USERNAME']
os.environ['REDIS_PASSWORD'] = app.config['REDIS_PASSWORD']
os.environ['REDIS_HOST'] = app.config['REDIS_HOST']
os.environ['REDIS_PORT'] = app.config['REDIS_PORT']
Copy link
Contributor

Choose a reason for hiding this comment

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

It sounds kind of looped dependencies, as it's setting the configs from app configs to system envs, while the app configs are also read from system envs. No a good idea change system envs at runtime for satisfying the extentionext_redis's need.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
💪 enhancement New feature or request size:M This PR changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants