Skip to content

Conversation

@chenzikun
Copy link

@chenzikun chenzikun commented May 8, 2025

  • cluster已经过测试,sentinel暂未测试

feature-redis cluster & sentinel

fix #147

@chenzikun chenzikun mentioned this pull request May 8, 2025
Copy link
Contributor

@Yeuoly Yeuoly left a comment

Choose a reason for hiding this comment

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

Please use English comments instead

@Yeuoly Yeuoly changed the title [add] redis新增cluster与sentinel [feat] support cluster and sentinel deployment for redis May 9, 2025
@erigo
Copy link
Contributor

erigo commented May 9, 2025

I suggest using redis.UniversalClient because it supports direct connections, Sentinel mode, and cluster mode, which can simplify configuration management.

I've created a PR at #276 to implement support for Sentinel mode. This will enable us to use Redis more reliably in the production environment.

@chenzikun chenzikun force-pushed the feat/redis-cluster-sentinel branch from 0c348b8 to fba8571 Compare May 10, 2025 10:17
@chenzikun
Copy link
Author

@Yeuoly [done] convert chinese comment to english, pls check!

@chenzikun chenzikun requested a review from Yeuoly May 11, 2025 15:14
@Yeuoly
Copy link
Contributor

Yeuoly commented May 14, 2025

I've reviewed #276, I prefer to use redis.UniversalClient as the client so that this is no need to maintain a interface like what you do, @chenzikun @erigo , how do you like?

@erigo
Copy link
Contributor

erigo commented May 14, 2025

I suggest adding support for Redis cluster based on #276.

@AkisAya
Copy link
Contributor

AkisAya commented May 15, 2025

i think sentinel mode and cluster mode of reids subpub will not work as expected. when there is a failure of a redis instance, pubsub connection of failed redis will not automatically change to another redis instance.
it's better to split the cache ability to different interfaces and carefully implement different part. #189

@erigo
Copy link
Contributor

erigo commented May 15, 2025

Thanks for pointing this out — I agree with your observation that Redis Sentinel and Cluster modes do not automatically recover Pub/Sub connections after failover, which makes Redis Pub/Sub unsuitable for high-availability scenarios.

However, I would respectfully disagree with the idea of “splitting the cache ability into different interfaces.”
In practice, it's often better to unify Redis access through a single interface using redis.UniversalClient, which allows seamless switching between standalone, Sentinel, and Cluster modes. This approach simplifies the codebase, avoids redundant abstractions, and still gives us flexibility for future changes.

As for Pub/Sub, I agree that Redis is not the right tool for reliable messaging. For systems that depend on high-availability message delivery, dedicated message queues like Kafka or NATS are far more suitable.

So in short: use redis.UniversalClient to unify caching access, but avoid Redis Pub/Sub for HA-critical workflows.

@AkisAya
Copy link
Contributor

AkisAya commented May 19, 2025

use redis.UniversalClient to unify caching access

this will make it hard to extend different caching storages. e.g. our redis cluster implemention is not opensource RedisCluster or Sentinel

but avoid Redis Pub/Sub for HA-critical workflows

agree. and it's better to split the current responsibilities of the cache into a cache interface and pubsub

@Yeuoly
Copy link
Contributor

Yeuoly commented May 20, 2025

use redis.UniversalClient to unify caching access

this will make it hard to extend different caching storages. e.g. our redis cluster implemention is not opensource RedisCluster or Sentinel

but avoid Redis Pub/Sub for HA-critical workflows

agree. and it's better to split the current responsibilities of the cache into a cache interface and pubsub

I agree splitting them into different interfaces would make cache more extendable, but just at this point, Redis is the production standard for almost all the software, adding support for other cache is not a priority I guess

@chenzikun chenzikun force-pushed the feat/redis-cluster-sentinel branch from fba8571 to 8953de4 Compare May 22, 2025 10:31
@chenzikun
Copy link
Author

@Yeuoly ok.I had change my code.redis cluster base on redis.UniversalClient, pls check!

@zhuzihao-hz
Copy link

Hi👋 , I tried UniversalClient and UniversalClient, but got the following error while running

[ERROR] PluginDaemonInternalServerError: redis: Watch requires at least one key
goroutine 1575 [running]:

Is this related to this func client.Watch(ctx, func(tx *redis.Tx) ?

Sorry, I'm not very familiar with the Redis API.

@JohnMin-ai
Copy link

JohnMin-ai commented May 27, 2025

@zhuzihao-hz @chenzikun
使用redis集群,本地启动dify调试,会向plugin发送请求:response = self._request_with_plugin_daemon_response("POST", f"plugin/{tenant_id}/debugging/key", Response),然后plugin会报错:[ERROR]PluginDaemonInternalServerError: redis: Watch requires at least one key

定位到 redis.go 文件 Transaction方法里面的报错位置为:
return client.Watch(ctx, func(tx *redis.Tx) error {
原因为:
client.Watch(ctx, fn) //缺少 keys 参数,导致 Redis 报错。

高版本的redis,比如:go-redis v9.5.x ,Watch() 方法源码中仍然保留以下判断逻辑
func (c *Client) Watch(ctx context.Context, fn func(*Tx) error, keys ...string) error {
if len(keys) == 0 {
return errors.New("redis: Watch requires at least one key")
}
...
}

@chenzikun
Copy link
Author

@zhuzihao-hz @chenzikun 使用redis集群,本地启动dify调试,会向plugin发送请求:response = self._request_with_plugin_daemon_response("POST", f"plugin/{tenant_id}/debugging/key", Response),然后plugin会报错:[ERROR]PluginDaemonInternalServerError: redis: Watch requires at least one key

定位到 redis.go 文件 Transaction方法里面的报错位置为: return client.Watch(ctx, func(tx *redis.Tx) error { 原因为: client.Watch(ctx, fn) //缺少 keys 参数,导致 Redis 报错。

高版本的redis,比如:go-redis v9.5.x ,Watch() 方法源码中仍然保留以下判断逻辑 func (c *Client) Watch(ctx context.Context, fn func(*Tx) error, keys ...string) error { if len(keys) == 0 { return errors.New("redis: Watch requires at least one key") } ... }

请问你能提供更多的上下文吗,我这边未出现你说情况,这种是集群模式导致的,还是redis本身存在这种错误;我这边可以修改Transaction这个函数,但是太确认是否会引起其他问题,因为我没有办法在所有的环境中验证。

@JohnMin-ai

@dosubot dosubot bot added the size:M This PR changes 30-99 lines, ignoring generated files. label Dec 17, 2025
@chenzikun chenzikun force-pushed the feat/redis-cluster-sentinel branch from 3bd17fb to 0d2966b Compare December 17, 2025 06:40
@chenzikun
Copy link
Author

@zhuzihao-hz @chenzikun 使用redis集群,本地启动dify debug,会向plugin发送请求:response = self._request_with_plugin_daemon_response("POST", f"plugin/{tenant_id}/debugging/key", Response),然后plugin会报错:[ERROR]PluginDaemonInternalServerError: redis: Watch require at least one key

定位到redis.go文件Transaction方法里面的报错位置为: return client.Watch(ctx, func(tx *redis.Tx) error { 原因为: client.Watch(ctx, fn) // 缺少keys参数,导致Redis报错。

高版本的redis,比如:go-redis v9.5.x ,Watch()方法源码中仍然保留以下判断逻辑 func (c *Client) Watch(ctx context.Context, fn func(*Tx) error,keys ...string) error { if len(keys) == 0 { return error.New("redis: Watch需要至少一个key") } ... }

fixed this error!

@chenzikun
Copy link
Author

@Yeuoly pls review this feature

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:M This PR changes 30-99 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

REDIS SENTINELS

6 participants