feat: add ReplicaOnly support for cluster client #346

NeoHuang · 2023-08-25T15:53:38Z

added ReplicaOnly support for cluster client.

tested with a cluster with two shards, 3 nodes each.

@rueian could you let me know if this implementation makes sense? I'll add tests later

codecov-commenter · 2023-08-25T16:10:22Z

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.01% 🎉

Comparison is base (92d534f) 97.60% compared to head (345c26d) 97.62%.
Report is 6 commits behind head on main.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #346      +/-   ##
==========================================
+ Coverage   97.60%   97.62%   +0.01%     
==========================================
  Files          75       75              
  Lines       30408    30429      +21     
==========================================
+ Hits        29681    29705      +24     
+ Misses        612      610       -2     
+ Partials      115      114       -1

Files Changed	Coverage Δ
client.go	`100.00% <100.00%> (ø)`
cluster.go	`98.56% <100.00%> (+<0.01%)`	⬆️
pipe.go	`100.00% <100.00%> (+0.29%)`	⬆️
rueidis.go	`100.00% <100.00%> (ø)`

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

rueian · 2023-08-25T16:37:13Z

cluster.go

+		if c.opt.ReplicaOnly {
+			if replicaAddr := g.pickReplica(); replicaAddr != "" {
+				cc = conns[replicaAddr]
+				result := cc.Do(context.Background(), cmds.ReadOnlyCmd)


Hi @NeoHuang,

Thank you for implementing the ReplicaOnly for the cluster client.

Your implementation generally looks good to me. However, setting the READONLY state right here is not a good idea. It will not only make this refresh function slow but also make it hard to finish its job.

I think it will be better to pipeline the READONLY right after the SELECT command.

Here for RESP3:

rueidis/pipe.go

Lines 173 to 175 in 6556f2f

if option.SelectDB != 0 {

init = append(init, []string{"SELECT", strconv.Itoa(option.SelectDB)})

}

And here for RESP2:

rueidis/pipe.go

Lines 247 to 249 in 6556f2f

if option.SelectDB != 0 {

init = append(init, []string{"SELECT", strconv.Itoa(option.SelectDB)})

}

We can probably rely on the option.ReplicaOnly flag to pipeline the READONLY command after these two blocks if the pipe is managed under a cluster client.

Hi @rueian , thanks for the feedback and I think it's a good idea. I'll make that change

actually if we call it here in newPipe, it would panic if the redis doesn't have the cluster mode enabled. but we don't seem to have any context that whether cluster mode is supported or not here as we always try cluster mode first?
any idea how we can get around? or we can simply return error if ReplicaOnly is set for non clustered redis?

Hi @NeoHuang,

We do try cluster mode first in the NewClient. If the server is not a cluster, then we treat it as a single redis:

rueidis/rueidis.go

Lines 303 to 306 in 19e115b

if client, err = newClusterClient(&option, makeConn); err != nil {

if len(option.InitAddress) == 1 && (err.Error() == redisErrMsgCommandNotAllow || strings.Contains(strings.ToUpper(err.Error()), "CLUSTER")) {

option.PipelineMultiplex = singleClientMultiplex(pmbk)

client, err = newSingleClient(&option, client.(*clusterClient).single(), makeConn)

Was the panic you mentioned caused by the return nil, err here?

rueidis/pipe.go

Lines 217 to 218 in 19e115b

p.Close()

return nil, err

We probably need to intercept that case and return p, err instead.

it's panic here

rueidis/rueidis.go

Line 306 in 19e115b

client, err = newSingleClient(&option, client.(*clusterClient).single(), makeConn)

the client is nil and not checked in this line. and yeah, the nil client was returned by your above code, but I am not sure if it's a good idea to intercept that specific case. I think I would just return error after this line

rueidis/rueidis.go

Line 303 in 19e115b

if client, err = newClusterClient(&option, makeConn); err != nil {

like this

if client, err = newClusterClient(&option, makeConn); err != nil { if client == nil || client == (*clusterClient)(nil) { return nil, err }

reason is that setting ReplicaOnly on single client is not expected anyway, I would rather throw this error instead of hiding it. also it would add more graceful handling of nil client cases for other scenarios. WDYT? :)

Hi @NeoHuang,

I now think, instead of returning p, err, we can just simply ignore the error caused by READONLY.

Because if the target server belongs to a redis cluster, the READONLY command will succeed, and if the target server doesn't belong to a cluster, the READONLY command will have no effect.

So, we can safely pipeline the READONLY command if option.ReplicaOnly is set without checking its response.

For throwing error in the case of ReplicaOnly+singleClient, we can do that before these line:

rueidis/rueidis.go

Lines 305 to 306 in 19e115b

option.PipelineMultiplex = singleClientMultiplex(pmbk)

client, err = newSingleClient(&option, client.(*clusterClient).single(), makeConn)

NeoHuang · 2023-08-28T14:08:30Z

@rueian thanks. updated the code according to the discussions and added tests

rueian · 2023-08-28T14:37:09Z

pipe.go

@@ -204,6 +209,10 @@ func _newPipe(connFn func() (net.Conn, error), option *ClientOption, r2ps bool)
 				err = r.Error()
 			}
 			if err != nil {
+				if readOnlyCmdIndex >= 0 && i == readOnlyCmdIndex {


Maybe this way is better and we don't have to introduce the readOnlyCmdIndex:

Suggested change

if readOnlyCmdIndex >= 0 && i == readOnlyCmdIndex {

if init[i][0] == "READONLY" {

rueian · 2023-08-28T15:27:31Z

cluster.go

+func (g group) pickReplica() string {
+	// no replica exists
+	if len(g.nodes) <= 1 {
+		return ""


Hi @NeoHuang,

Almost there. I find that this line is not covered by tests:

However, I am also wondering if the double-checking logic len(g.nodes) <= 1 and replicaAddr != "" could be merged?

For example just:

addr := master if c.opt.ReplicaOnly && len(g.nodes) > 1 { addr = g.nodes[1+rand.Intn(len(g.nodes)-1)] }

NeoHuang · 2023-08-28T16:03:38Z

btw, CLUSTER SLOTS seems to be deprecated. do we have plan to implement CLUSTER SHARDS

rueian · 2023-08-28T16:33:28Z

btw, CLUSTER SLOTS seems to be deprecated. do we have plan to implement CLUSTER SHARDS

Yes, we do need to implement CLUSTER SHARDS. We already have an issue opened for this #350. Given that CLUSTER SLOTS can still work for a long time, we have plenty of time on this.

rueian · 2023-08-28T16:44:21Z

Merged. Many thanks! @NeoHuang

rueian reviewed Aug 25, 2023

View reviewed changes

NeoHuang force-pushed the read_from_replica branch from dd8642a to 2aa436f Compare August 28, 2023 14:07

rueian reviewed Aug 28, 2023

View reviewed changes

NeoHuang force-pushed the read_from_replica branch from 2aa436f to 4f31803 Compare August 28, 2023 15:03

rueian reviewed Aug 28, 2023

View reviewed changes

Add ReplicaOnly support for cluster client

345c26d

NeoHuang force-pushed the read_from_replica branch from 4f31803 to 345c26d Compare August 28, 2023 15:46

rueian merged commit d17c53b into redis:main Aug 28, 2023

rueian mentioned this pull request Aug 29, 2023

Readonly command on Replica servers only #132

Closed

rueian changed the title ~~Add ReplicaOnly support for cluster client~~ feat: add ReplicaOnly support for cluster client Sep 1, 2023

rueian added the feature label Sep 1, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add ReplicaOnly support for cluster client #346

feat: add ReplicaOnly support for cluster client #346

NeoHuang commented Aug 25, 2023

codecov-commenter commented Aug 25, 2023 •

edited

Loading

rueian Aug 25, 2023

NeoHuang Aug 28, 2023

NeoHuang Aug 28, 2023 •

edited

Loading

rueian Aug 28, 2023

rueian Aug 28, 2023

NeoHuang Aug 28, 2023

rueian Aug 28, 2023

rueian Aug 28, 2023

rueian Aug 28, 2023

rueian Aug 28, 2023

NeoHuang commented Aug 28, 2023

rueian Aug 28, 2023

rueian Aug 28, 2023

NeoHuang commented Aug 28, 2023

rueian commented Aug 28, 2023

rueian commented Aug 28, 2023

	if option.SelectDB != 0 {
	init = append(init, []string{"SELECT", strconv.Itoa(option.SelectDB)})
	}

	if client, err = newClusterClient(&option, makeConn); err != nil {
	if len(option.InitAddress) == 1 && (err.Error() == redisErrMsgCommandNotAllow \|\| strings.Contains(strings.ToUpper(err.Error()), "CLUSTER")) {
	option.PipelineMultiplex = singleClientMultiplex(pmbk)
	client, err = newSingleClient(&option, client.(*clusterClient).single(), makeConn)

	if readOnlyCmdIndex >= 0 && i == readOnlyCmdIndex {
	if init[i][0] == "READONLY" {

feat: add ReplicaOnly support for cluster client #346

feat: add ReplicaOnly support for cluster client #346

Conversation

NeoHuang commented Aug 25, 2023

codecov-commenter commented Aug 25, 2023 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NeoHuang Aug 28, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NeoHuang commented Aug 28, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NeoHuang commented Aug 28, 2023

rueian commented Aug 28, 2023

rueian commented Aug 28, 2023

codecov-commenter commented Aug 25, 2023 •

edited

Loading

NeoHuang Aug 28, 2023 •

edited

Loading