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.

Sign up for GitHub

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: replica selector #692

Merged
merged 9 commits into from
Dec 13, 2024
+1,454 −33
Merged

feat: replica selector #692

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
3185bbc
feat: add reader node selector
proost Dec 11, 2024
9499b9b
doc: add comments
proost Dec 11, 2024
5261cb1
fix: wrong error message
proost Dec 11, 2024
a7062fd
test: add send read-only command to reader node
proost Dec 11, 2024
76f58a3
refactor: use sendtoreplicas and replicaselector
proost Dec 12, 2024
90387d2
test: remove duplicated cases
proost Dec 12, 2024
9a7e752
Merge branch 'main' of https://github.com/redis/rueidis into feat-rea…
proost Dec 13, 2024
91bbcf3
test: remove cases
proost Dec 13, 2024
50fad71
perf: introduce nodes
proost Dec 13, 2024
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
32 changes: 30 additions & 2 deletions cluster.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,8 @@ import (
var ErrNoSlot = errors.New("the slot has no redis node")
var ErrReplicaOnlyConflict = errors.New("ReplicaOnly conflicts with SendToReplicas option")
var ErrInvalidShardsRefreshInterval = errors.New("ShardsRefreshInterval must be greater than or equal to 0")
var ErrReplicaOnlyConflictWithReplicaSelector = errors.New("ReplicaOnly conflicts with ReplicaSelector option")
var ErrSendToReplicasNotSet = errors.New("SendToReplicas must be set when ReplicaSelector is set")

type clusterClient struct {
pslots [16384]conn
Expand All @@ -41,6 +43,10 @@ type connrole struct {
replica bool
}

var replicaOnlySelector = func(_ uint16, replicas []ReplicaInfo) int {
return util.FastRand(len(replicas))
}

func newClusterClient(opt *ClientOption, connFn connFn, retryer retryHandler) (*clusterClient, error) {
client := &clusterClient{
cmd: cmds.NewBuilder(cmds.InitSlot),
Expand All @@ -55,6 +61,16 @@ func newClusterClient(opt *ClientOption, connFn connFn, retryer retryHandler) (*
if opt.ReplicaOnly && opt.SendToReplicas != nil {
return nil, ErrReplicaOnlyConflict
}
if opt.ReplicaOnly && opt.ReplicaSelector != nil {
return nil, ErrReplicaOnlyConflictWithReplicaSelector
}
if opt.ReplicaSelector != nil && opt.SendToReplicas == nil {
return nil, ErrSendToReplicasNotSet
}

if opt.SendToReplicas != nil && opt.ReplicaSelector == nil {
opt.ReplicaSelector = replicaOnlySelector
}

if opt.SendToReplicas != nil {
rOpt := *opt
Expand Down Expand Up @@ -236,15 +252,27 @@ func (c *clusterClient) _refresh() (err error) {
pslots[i] = conns[g.nodes[1+util.FastRand(nodesCount-1)]].conn
}
}
case c.rOpt != nil: // implies c.opt.SendToReplicas != nil
case c.rOpt != nil:
if len(rslots) == 0 { // lazy init
rslots = make([]conn, 16384)
}
if len(g.nodes) > 1 {
n := len(g.nodes) - 1
replicas := make([]ReplicaInfo, 0, n)
for _, addr := range g.nodes[1:] {
Copy link
Collaborator

@rueian rueian Dec 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Can we avoid this allocation by replacing g.nodes from []string to []ReplicaInfo?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I already consider it. But i didn't. Because

  1. g.nodes has primary node too. so ReplicaInfo slice type is incorrect. It can be misleading to contributors.
  2. user can change value each ReplicaInfo. Changing ReplicaInfo is not problem to rueidis. but there can be side effect in the user side.

Because of duplicated allocation, i also considering reusing ReplicaInfo slice. But problem is If user holds ReplicaInfo slice in the ReplicaSelector it can be problem.

I prefer reusing ReplicaInfo slice if should reduce allocation. And in this case, we should add comment that user must not store reference ReplicaInfo slice.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hi @proost,

Great considerations!

  1. If naming is the concern, we can use some sort of the type alias. We can only expose ReplicaInfo to users but use another name internally.
  2. We can add comments on the selector function to tell the user should not change values in the slice.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

50fad71

OK, I changed it.

replicas = append(replicas, ReplicaInfo{Addr: addr})
}

for _, slot := range g.slots {
for i := slot[0]; i <= slot[1]; i++ {
pslots[i] = conns[master].conn
rslots[i] = conns[g.nodes[1+util.FastRand(len(g.nodes)-1)]].conn

rIndex := c.opt.ReplicaSelector(uint16(i), replicas)
if rIndex >= 0 && rIndex < n {
rslots[i] = conns[g.nodes[1+rIndex]].conn
} else {
rslots[i] = conns[master].conn
}
}
}
} else {
Expand Down
Loading
Loading