fix: send readonly command #430

proost · 2023-12-19T14:05:55Z

I missed to send "READONLY" command for replica connection.

I want to integrate "READONLY" command with pipeline constructor, but it quite hard. has any idea?

codecov-commenter · 2023-12-19T14:18:59Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (65f705a) 95.97% compared to head (87e2b39) 95.96%.
Report is 3 commits behind head on main.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #430      +/-   ##
==========================================
- Coverage   95.97%   95.96%   -0.02%     
==========================================
  Files          77       77              
  Lines       32403    32414      +11     
==========================================
+ Hits        31099    31106       +7     
- Misses       1110     1113       +3     
- Partials      194      195       +1

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

rueian · 2023-12-19T16:53:34Z

Hi @proost, I think we should reuse this block:

rueidis/pipe.go

Lines 273 to 275 in 65f705a

    
           if option.ReplicaOnly && option.Sentinel.MasterSet == "" { 
        
           	init = append(init, []string{"READONLY"}) 
        
           }

One more thing needs to be considered: A replica can be promoted to primary, meaning we should reset the 'READONLY' state with the READWRITE command.

proost · 2023-12-22T07:00:32Z

@rueian
yes, you are right. replica can be promoted to primary. but it is hard to notice that replica is promoted.
I thinkg that adding ReplicaConnectionReadOnly to rueidis.ClientOption and sending "READONLY" command in _refresh can be better. because user knows when replica connection can be read-only connection. how do you think change like this?

rueian · 2023-12-22T17:55:46Z

Hi @proost,

I don’t think we need a new option. How about we modify the ReplicaOnly to true only for replica connections from the refresh function? And if we find a replica connection is promoted when refreshing, then we open a new one without ReplicaOnly.

proost · 2023-12-23T07:51:00Z

@rueian

I change it. is okay to use ReplicaOnly? because it is breaking change.

rueian · 2023-12-23T08:53:25Z

cluster.go

+			go func(cc conn) {
+				defer wg.Done()
+
+				timeout := c.opt.Dialer.Timeout
+				if timeout <= 0 {
+					timeout = DefaultDialTimeout
+				}
+
+				ctx, cancel := context.WithTimeout(context.Background(), timeout)
+				defer cancel()
+
+				cc.Do(ctx, cmds.NewCompleted([]string{"READONLY"})) // ignore error
+			}(cc)


Hi @proost,

Thank you for the changes, but this goroutine looks wired to me and I don't expect there will be breaking change by reusing the ReplicaOnly. I am afraid we have some misunderstanding here.

What I previously suggested is that the pipe will send READONLY command already if its ReplicaOnly is true.
So I think we can reuse this behavior by modifying the c.connFn(addr, c.opt) in the refresh function, and then the above goroutine is not necessary.

Given that the signature of the connFn is func(dst string, opt *ClientOption) conn which accepts a ClientOption pointer, I think we can copy the c.opt to c.optReplica with the ReplicaOnly set to true and then we can use c.connFn(addr, c.optReplica) for replica connections.

98040d7

@rueian
I'm sorry about serious misunderstanding. I figured out.

rueian · 2023-12-23T13:40:57Z

cluster.go

-			conns[addr] = connrole{conn: c.connFn(addr, c.opt), replica: true}
+			var cc conn
+			if c.opt.ReplicaOnly {
+				opt := *c.opt


I am still a little confused. So now, are we requiring users to set ReplicaOnly as well even if they use SendToReplicas?

I thought we would only set ReplicaOnly transparently for users here to the opt.

fix it. 3ec3548

when SendToReplicas is configured and ReplicaOnly is true,

connection to master: do not send "READONLY"

connection to replica: send "READONLY"

when SendToReplicas is configured and ReplicaOnly is false,

connection to master: do not send "READONLY"

connection to replica: do not send "READONLY"

when SendToReplicas is not configured and ReplicaOnly is true,

connection to master: send "READONLY"

Why we don't send "READONLY" to replicas when SendToReplicas is configured and ReplicaOnly is false?

Isn't it the original issue you want to fix?

cd6a674

Yes, it doesn't matter HOW. i just want to send "READONLY" command to replica connections automatically. but if you think read-only mode as a default is better than i changed it.

rueian · 2023-12-24T05:49:41Z

cluster.go

+		opt := *c.opt
+		if c.opt.SendToReplicas != nil {
+			opt.ReplicaOnly = false
+		}
+		conns[master] = connrole{conn: c.connFn(master, &opt), replica: false}


The current code above will allocate a copy of c.opt on the heap every time.
We should do it better by preparing the copied opt once in advance.

cd6a674

yes. you're right.

rueian · 2023-12-24T09:05:35Z

cluster.go

+	masterOpt := *c.opt
+	if c.opt.SendToReplicas != nil {
+		masterOpt.ReplicaOnly = false
+	}
+	replicaOpt := *c.opt
+	if c.opt.SendToReplicas != nil {
+		replicaOpt.ReplicaOnly = !c.opt.ReplicaOnly
+	}


It is better now. But it still allocates masterOpt and replicaOpt in every refresh.
We can make it even better by storing a c.replicaOpt in the newClusterClient.

i just want to send "READONLY" command to replica connections automatically.

Agreed. So, how about this:

when SendToReplicas is configured and ReplicaOnly is true,

return ErrReplicaOnlyConflict

when SendToReplicas is configured and ReplicaOnly is false,

connection to master: use c.connFn(addr, c.opt)

connection to replica: use c.connFn(addr, c.replicaOpt)

when SendToReplicas is not configured and ReplicaOnly is true,

connection to all nodes: use c.connFn(addr, c.opt), we don't need c.replicaOpt in this case.

5702ce3

Good! i changed it.

rueian · 2023-12-24T14:45:31Z

cluster.go

@@ -241,7 +254,8 @@ func (c *clusterClient) _refresh() (err error) {

 	c.mu.RLock()
 	for addr, cc := range c.conns {
-		if fresh, ok := conns[addr]; ok {
+		fresh, ok := conns[addr]
+		if ok && cc.replica == fresh.replica {


Suggested change

if ok && cc.replica == fresh.replica {

if ok && (cc.replica == fresh.replica || c.opt.SendToReplicas == nil) {

We can still reuse the conn if c.opt.SendToReplicas is not configured.

Hi @proost, thank you very much! It looks great!

fix: send readonly command

7759b3a

proost added 4 commits December 23, 2023 15:28

refactor: rollback

92288cc

refactor: rollback

519cd72

BREAKING: remove error

0231a44

refactor: send readonly command in _refresh

a2a1aa8

rueian reviewed Dec 23, 2023

View reviewed changes

proost added 2 commits December 23, 2023 22:24

refactor: use shallow copy

98040d7

style: unify style

f328c8f

proost requested a review from rueian December 23, 2023 13:27

rueian reviewed Dec 23, 2023

View reviewed changes

proost added 2 commits December 23, 2023 22:45

refactor: call conn function

ccd4a28

fix: wrong code

3ec3548

proost requested a review from rueian December 23, 2023 13:58

rueian reviewed Dec 24, 2023

View reviewed changes

refactor: default to read-only mode

cd6a674

proost requested a review from rueian December 24, 2023 08:51

rueian reviewed Dec 24, 2023

View reviewed changes

proost added 2 commits December 24, 2023 22:11

refactor: change more simpler

5702ce3

refactor: rollback

1375321

proost requested a review from rueian December 24, 2023 13:54

rueian reviewed Dec 24, 2023

View reviewed changes

fix: check send replicas

87e2b39

proost requested a review from rueian December 25, 2023 12:44

rueian added the enhancement New feature or request label Dec 25, 2023

rueian merged commit b611f0c into redis:main Dec 25, 2023

proost deleted the fix-readonly branch December 26, 2023 02:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: send readonly command #430

fix: send readonly command #430

proost commented Dec 19, 2023

codecov-commenter commented Dec 19, 2023 •

edited

Loading

rueian commented Dec 19, 2023

proost commented Dec 22, 2023 •

edited

Loading

rueian commented Dec 22, 2023

proost commented Dec 23, 2023

rueian Dec 23, 2023

rueian Dec 23, 2023

proost Dec 23, 2023

rueian Dec 23, 2023

proost Dec 23, 2023

rueian Dec 23, 2023

proost Dec 24, 2023

rueian Dec 24, 2023

proost Dec 24, 2023

rueian Dec 24, 2023

rueian Dec 24, 2023

proost Dec 24, 2023

rueian Dec 24, 2023

rueian Dec 24, 2023

proost Dec 25, 2023

rueian Dec 25, 2023

	if ok && cc.replica == fresh.replica {
	if ok && (cc.replica == fresh.replica \|\| c.opt.SendToReplicas == nil) {

fix: send readonly command #430

fix: send readonly command #430

Conversation

proost commented Dec 19, 2023

codecov-commenter commented Dec 19, 2023 • edited Loading

Codecov Report

rueian commented Dec 19, 2023

proost commented Dec 22, 2023 • edited Loading

rueian commented Dec 22, 2023

proost commented Dec 23, 2023

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

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

codecov-commenter commented Dec 19, 2023 •

edited

Loading

proost commented Dec 22, 2023 •

edited

Loading