Enhance Redis client to support Sentinel high availability#4559
Open
dyunwei wants to merge 2 commits intoQuantumNous:mainfrom
Open
Enhance Redis client to support Sentinel high availability#4559dyunwei wants to merge 2 commits intoQuantumNous:mainfrom
dyunwei wants to merge 2 commits intoQuantumNous:mainfrom
Conversation
Previously, `InitRedisClient` lacked support for Redis Sentinel. This update allows the application to connect to a Sentinel cluster for automatic failover. Configuration is driven by the following new environment variables: * REDIS_SENTINEL_ADDRS (e.g., "192.168.1.10:26379,192.168.1.20:26379...") * REDIS_MASTER_NAME (e.g., "mymaster") * REDIS_PASSWORD * REDIS_SENTINEL_USERNAME (Optional) * REDIS_SENTINEL_PASSWORD (Optional)
Contributor
WalkthroughThe Changes
Sequence Diagram(s)sequenceDiagram
participant App as Application
participant Init as InitRedisClient
participant Env as Environment
participant Sentinel as Redis Sentinel
participant Redis as Redis Instance
App->>Init: Call InitRedisClient()
Init->>Env: Read REDIS_SENTINEL_ADDRS / REDIS_CONN_STRING
alt Sentinel mode (REDIS_SENTINEL_ADDRS present)
Env-->>Init: sentinel addrs, master name, creds
Init->>Sentinel: Create failover client (pool, auth, DB)
Sentinel->>Redis: Discover master endpoint
Init->>Redis: Ping
Redis-->>Init: Pong
else Standalone mode
Env-->>Init: connection string, creds
Init->>Redis: Create standalone client (pool, auth, DB)
Init->>Redis: Ping
Redis-->>Init: Pong
end
Init-->>App: Redis client ready
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Review rate limit: 7/8 reviews remaining, refill in 7 minutes and 30 seconds.Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@common/redis.go`:
- Around line 26-29: InitRedisClient currently only flips RedisEnabled=false
when env vars are missing, which can leak a previous RDB client and never reset
the flag on subsequent successful inits; update InitRedisClient to reset globals
at start (set RedisEnabled=false and RDB=nil), then only set RedisEnabled=true
after successfully creating and assigning the new Redis client (RDB), and ensure
any failure path also clears RDB (nil) so no previous client leaks; reference
symbols: InitRedisClient, RedisEnabled, RDB.
- Around line 66-82: The Sentinel (failover) branch builds
redis.NewFailoverClient without setting the DB index, so deployments using a
non-zero Redis DB silently connect to DB 0; update the FailoverOptions passed to
redis.NewFailoverClient to include the same DB index used in the standalone path
(set the DB field). To do this, extract the DB index the standalone path gets
(e.g., by calling redis.ParseURL(connString) or reading the existing DB
variable) and add DB: <thatIndex> to the redis.FailoverOptions passed to
redis.NewFailoverClient (symbols to change: redis.NewFailoverClient,
redis.FailoverOptions, MasterName, SentinelAddrs, SentinelUsername,
SentinelPassword, Password, PoolSize, add DB).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
增加一个 REDIS_DB 环境变量，为哨兵模式指定 DB 索引。 Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
common/redis.go (1)
26-29:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winReset and republish the Redis globals per init call.
This still leaves
RedisEnabled/RDBcarrying state across repeatedInitRedisClientcalls: after one disabled init, a later successful init never flipsRedisEnabledback totrue, and disabling Redis later can keep an old client reachable throughRDB. Reset the globals up front and only assign the new client afterPingsucceeds.Suggested fix
func InitRedisClient() (err error) { + RedisEnabled = false + RDB = nil + if os.Getenv("REDIS_CONN_STRING") == "" && os.Getenv("REDIS_SENTINEL_ADDRS") == "" { - RedisEnabled = false SysLog("REDIS_CONN_STRING and REDIS_SENTINEL_ADDRS not set, Redis is not enabled") return nil } @@ - if sentinelAddrs != "" { + var client *redis.Client + if sentinelAddrs != "" { @@ - RDB = redis.NewFailoverClient(&redis.FailoverOptions{ + client = redis.NewFailoverClient(&redis.FailoverOptions{ MasterName: masterName, SentinelAddrs: addrs, SentinelUsername: sentinelUsername, SentinelPassword: sentinelPassword, Password: password, DB: db, PoolSize: poolSize, }) } else { @@ - RDB = redis.NewClient(opt) + client = redis.NewClient(opt) } @@ - _, err = RDB.Ping(ctx).Result() + _, err = client.Ping(ctx).Result() if err != nil { FatalLog("Redis ping test failed: " + err.Error()) } + RDB = client + RedisEnabled = true SysLog("Redis connection test passed") return err }Also applies to: 67-84, 90-95
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@common/redis.go` around lines 26 - 29, In InitRedisClient reset RedisEnabled = false and RDB = nil at the start to clear prior state, then attempt to create the new client (from connection-string or sentinel code paths) but do not assign it to the global RDB or set RedisEnabled until after client.Ping(ctx) succeeds; if Ping succeeds, close any previous client (if non-nil), set RDB to the new client and RedisEnabled = true, and if Ping fails ensure the new client is closed and globals remain disabled; update all branches (the direct conn-string and sentinel branches used in InitRedisClient and the other similar blocks) to follow this pattern and emit SysLog messages accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@common/redis.go`:
- Around line 26-29: In InitRedisClient reset RedisEnabled = false and RDB = nil
at the start to clear prior state, then attempt to create the new client (from
connection-string or sentinel code paths) but do not assign it to the global RDB
or set RedisEnabled until after client.Ping(ctx) succeeds; if Ping succeeds,
close any previous client (if non-nil), set RDB to the new client and
RedisEnabled = true, and if Ping fails ensure the new client is closed and
globals remain disabled; update all branches (the direct conn-string and
sentinel branches used in InitRedisClient and the other similar blocks) to
follow this pattern and emit SysLog messages accordingly.
Author
|
虽然接受了coderabbitai新增指定数据库索引的提议，但强烈建议只用 DB 0（即不要配置环境变量 REDIS_DB ）。此外，基于最小修改原则，本次更新并没有修改以下变量的声明。 var RDB *redis.Client
var RedisEnabled = true沿用了已有版本关于是否启用Redis的逻辑： if os.Getenv("REDIS_CONN_STRING") == "" && os.Getenv("REDIS_SENTINEL_ADDRS") == "" {
RedisEnabled = false
SysLog("REDIS_CONN_STRING and REDIS_SENTINEL_ADDRS not set, Redis is not enabled")
return nil
} |
Author
|
@Calcium-Ion 麻烦抽空review一下，第一次在这个项目提交代码，不清楚流程对不对。。。 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Previously,
InitRedisClientlacked support for Redis Sentinel. This update allows the application to connect to a Sentinel cluster for automatic failover.Configuration is driven by the following new environment variables:
📝 变更描述 / Description
官方文档 中描述了支持Redis的哨兵模式，但经过检查后，实际代码中并没支持。本次修改了common/redis.go 文件中 InitRedisClient 函数，目的就是为了支持该项功能。改动由copilot辅助完成，且由人工进行了review，并进行了集群测试。以下为该功能的新增的环境变量配置说明，后续会更新进入文档。
🚀 变更类型 / Type of change
🔗 关联任务 / Related Issue
✅ 提交前检查项 / Checklist
Bug fix，我已提交或关联对应 Issue，且不会将设计取舍、预期不一致或理解偏差直接归类为 bug。📸 运行证明 / Proof of Work
Summary by CodeRabbit
New Features
Improvements