Skip to content

Make FailoverClient(sentinel mode) more robust #1655

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

Merged
merged 19 commits into from
Feb 10, 2021
Merged
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Refine code
  • Loading branch information
wangrzneu committed Feb 9, 2021
commit a731cce922b790abbb8c5bfac1afebfc8548ca43
56 changes: 27 additions & 29 deletions sentinel.go
Original file line number Diff line number Diff line change
Expand Up @@ -36,10 +36,12 @@ 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
// Use slaves disconnected with master when cannot get connected slaves
// Now, this option only works in RandomSlaveAddr function.
AllowDisconnectedSlaves bool
UseDisconnectedSlaves bool

// Client queries sentinels in a random order
QuerySentinelRandomly bool
// Following options are copied from Options struct.

Dialer func(ctx context.Context, network, addr string) (net.Conn, error)
Expand Down Expand Up @@ -441,20 +443,16 @@ func (c *sentinelFailover) RandomSlaveAddr(ctx context.Context) (string, error)
return "", errors.New("opt is nil")
}

var addresses []string
var err error
addresses, err := c.slaveAddrs(ctx, false)
if err != nil {
return "", err
}

for _, allowDisconnected := range []bool{false, true} {
if allowDisconnected && !c.opt.AllowDisconnectedSlaves {
continue
}
addresses, err = c.slaveAddrs(ctx, allowDisconnected)
if len(addresses) == 0 && c.opt.UseDisconnectedSlaves {
addresses, err = c.slaveAddrs(ctx, true)
if err != nil {
return "", err
}
if len(addresses) > 0 {
break
}
}

if len(addresses) == 0 {
Expand Down Expand Up @@ -485,7 +483,11 @@ func (c *sentinelFailover) MasterAddr(ctx context.Context) (string, error) {
}
_ = c.closeSentinel()
}

if c.opt.QuerySentinelRandomly {
rand.Shuffle(len(c.sentinelAddrs), func(i, j int) {
c.sentinelAddrs[i], c.sentinelAddrs[j] = c.sentinelAddrs[j], c.sentinelAddrs[i]
})
}
for i, sentinelAddr := range c.sentinelAddrs {
sentinel := NewSentinelClient(c.opt.sentinelOptions(sentinelAddr))

Expand All @@ -508,7 +510,7 @@ func (c *sentinelFailover) MasterAddr(ctx context.Context) (string, error) {
return "", errors.New("redis: all sentinels specified in configuration are unreachable")
}

func (c *sentinelFailover) slaveAddrs(ctx context.Context, allowDisconnected bool) ([]string, error) {
func (c *sentinelFailover) slaveAddrs(ctx context.Context, useDisconnected bool) ([]string, error) {
c.mu.RLock()
sentinel := c.sentinel
c.mu.RUnlock()
Expand All @@ -529,18 +531,13 @@ func (c *sentinelFailover) slaveAddrs(ctx context.Context, allowDisconnected boo
return addrs, nil
}
_ = c.closeSentinel()
} else {
// shuffle the sentinelAddrs to make the load more balanced
}
if c.opt.QuerySentinelRandomly {
rand.Shuffle(len(c.sentinelAddrs), func(i, j int) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should this code really be in else clause?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should this code really be in else clause?

I add QuerySentinelRandomly option to decide whether to shuffle.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

c.sentinelAddrs[i], c.sentinelAddrs[j] = c.sentinelAddrs[j], c.sentinelAddrs[i]
})
}

allowedFlags := make([]string, 0)
if allowDisconnected {
allowedFlags = append(allowedFlags, "disconnected")
}

var sentinelReachable bool

for i, sentinelAddr := range c.sentinelAddrs {
Expand All @@ -554,7 +551,7 @@ func (c *sentinelFailover) slaveAddrs(ctx context.Context, allowDisconnected boo
continue
}
sentinelReachable = true
addrs := parseSlaveAddrs(slaves, allowedFlags...)
addrs := parseSlaveAddrs(slaves, useDisconnected)
if len(addrs) == 0 {
continue
}
Expand Down Expand Up @@ -588,11 +585,10 @@ func (c *sentinelFailover) getSlaveAddrs(ctx context.Context, sentinel *Sentinel
c.opt.MasterName, err)
return []string{}
}
return parseSlaveAddrs(addrs)
return parseSlaveAddrs(addrs, false)
}

func parseSlaveAddrs(addrs []interface{}, allowedFlags ...string) []string {
downFlags := []string{"s_down", "o_down", "disconnected"}
func parseSlaveAddrs(addrs []interface{}, keepDisconnected bool) []string {
nodes := make([]string, 0, len(addrs))
for _, node := range addrs {
ip := ""
Expand All @@ -614,11 +610,13 @@ func parseSlaveAddrs(addrs []interface{}, allowedFlags ...string) []string {
}

for _, flag := range flags {
switch {
case contains(allowedFlags, flag):
continue
case contains(downFlags, flag):
switch flag {
case "s_down", "o_down":
isDown = true
case "disconnected":
if !keepDisconnected {
isDown = true
}
}
}

Expand Down