Make FailoverClient(sentinel mode) more robust #1655
Conversation
2. shuffle the sentinel addresses 3. query other sentinels when get empty slave addresses
# Conflicts: # sentinel.go
Dev See merge request gray-gateway/go-redis!2
Fix lint issue See merge request gray-gateway/go-redis!3
add comment See merge request gray-gateway/go-redis!4
checkout unrelated changes See merge request gray-gateway/go-redis!5
sentinel.go
Outdated
| var addresses []string | ||
| var err error | ||
|
|
||
| for _, allowDisconnected := range []bool{false, true} { |
There was a problem hiding this comment.
Can we simplify this? It is getting late here and I am having troubles trying to understand what this does...
If it is not possible - please add a comment explaining what is happening.
sentinel.go
Outdated
| @@ -553,9 +591,9 @@ func (c *sentinelFailover) getSlaveAddrs(ctx context.Context, sentinel *Sentinel | |||
| return parseSlaveAddrs(addrs) | |||
| } | |||
|
|
|||
| func parseSlaveAddrs(addrs []interface{}) []string { | |||
| func parseSlaveAddrs(addrs []interface{}, allowedFlags ...string) []string { | |||
There was a problem hiding this comment.
If I read this code correctly allowedFlags should be replaced with keepDisconnected bool.
| @@ -576,8 +614,10 @@ func parseSlaveAddrs(addrs []interface{}) []string { | |||
| } | |||
|
|
|||
| for _, flag := range flags { | |||
| switch flag { | |||
| case "s_down", "o_down", "disconnected": | |||
There was a problem hiding this comment.
Please keep this switch but add a separate clause like:
case "disconnected":
if !keepDisconnected {
isDown = true
}
| @@ -509,8 +529,20 @@ func (c *sentinelFailover) slaveAddrs(ctx context.Context) ([]string, error) { | |||
| return addrs, nil | |||
| } | |||
| _ = c.closeSentinel() | |||
| } else { | |||
| // shuffle the sentinelAddrs to make the load more balanced | |||
| rand.Shuffle(len(c.sentinelAddrs), func(i, j int) { | |||
There was a problem hiding this comment.
Should this code really be in else clause?
There was a problem hiding this comment.
Should this code really be in
elseclause?
I add QuerySentinelRandomly option to decide whether to shuffle.
There was a problem hiding this comment.
I am not sure this is an essential option so I probably going to remove it. Let me know if you think it is essential.
I guess your original code was correct and we only want to shuffle this list once.
sentinel.go
Outdated
| @@ -36,6 +36,10 @@ type FailoverOptions struct { | |||
| // Route all commands to slave read-only nodes. | |||
| SlaveOnly bool | |||
|
|
|||
| // Allow to access slaves disconnected with master when cannot get connected slaves | |||
| // Now, this option only works in RandomSlaveAddr function. | |||
| AllowDisconnectedSlaves bool | |||
There was a problem hiding this comment.
Allow is a bit vague here. I would rename to UseDisconnectedSlaves.
Also perhaps disconnected slaves should be used only when there are no healthy slaves?
There was a problem hiding this comment.
It is renamed to UseDisconnectedSlaves.
Refine code See merge request gray-gateway/go-redis!6
checkout unrelated changes See merge request gray-gateway/go-redis!8
|
@vmihailenco PTAL |
|
Looks good. I am going to tweak code a bit myself but but nothing big. |
Make FailoverClient(sentinel mode) more robust