valkey-io / valkey-go

A fast Golang Valkey client that supports Client Side Caching and Auto Pipelining.
Apache License 2.0
157 stars 10 forks source link

InitAddress issue when connecting to a valkey cluster using NewClient() #9

Open moment1027 opened 4 months ago

moment1027 commented 4 months ago

When connecting to a valkey cluster using NewClient(), if InitAddress is hostname rather than ip, the hostname and the ip for hostname are queried when Nodes() are called as shown below.

[InitValkey] InitValkey, conf:{Addr:[valkey-master-1:6379] } [InitValkey] Nodes Address:valkey-master-1:6379, Nodes:&{conn:0x400011e2c0 stop:0 cmd:{ks:16384} retry:true DisableCache:false}

[InitValkey] Nodes Address:172.19.0.10:6379, Nodes:&{conn:0x400052a2c0 stop:0 cmd:{ks:16384} retry:true DisableCache:false} <= valkey-master-1's IP address [InitValkey] Nodes Address:172.19.0.12:6379, Nodes:&{conn:0x400052a160 stop:0 cmd:{ks:16384} retry:true DisableCache:false} [InitValkey] Nodes Address:172.19.0.13:6379, Nodes:&{conn:0x400052a000 stop:0 cmd:{ks:16384} retry:true DisableCache:false}

[InitValkey] Nodes Address:172.19.0.9:6379, Nodes:&{conn:0x400052a0b0 stop:0 cmd:{ks:16384} retry:true DisableCache:false} [InitValkey] Nodes Address:172.19.0.11:6379, Nodes:&{conn:0x4000326000 stop:0 cmd:{ks:16384} retry:true DisableCache:false} [InitValkey] Nodes Address:172.19.0.14:6379, Nodes:&{conn:0x400052a210 stop:0 cmd:{ks:16384} retry:true DisableCache:false}

Therefore, when you query the Cluster Node using Nodes(), the data is duplicated. // master [Keys] address:valkey-master-1:6379, keys:[key_3] [Keys] address:172.19.0.10:6379, keys:[key_3] [Keys] address:172.19.0.12:6379, keys:[key_2] [Keys] address:172.19.0.13:6379, keys:[key_1 key_4 key_0]

// slave [Keys] address:172.19.0.9:6379, keys:[] [Keys] address:172.19.0.11:6379, keys:[] [Keys] address:172.19.0.14:6379, keys:[]

moment1027 commented 4 months ago

The cause is the code below. cluster.go, line:242

// make sure InitAddress always be present
for _, addr := range c.opt.InitAddress {
    if _, ok := conns[addr]; !ok {
        conns[addr] = connrole{
            conn: c.connFn(addr, c.opt),
        }
    }
}
rueian commented 4 months ago

Hi @moment1027, thanks for reporting! This is indeed caused by the code you mentioned and can be easily fixed by adding a new field to the connrole struct, for example:

// make sure InitAddress always be present
for _, addr := range c.opt.InitAddress {
    if _, ok := conns[addr]; !ok {
        conns[addr] = connrole{
            conn: c.connFn(addr, c.opt),
            hidden: true, // <- add this, and filter by this when using `Nodes()`
        }
    }
}

Would you like to open a PR to https://github.com/redis/rueidis?

moment1027 commented 3 months ago

Is there any particular reason for initaddress to be saved in conns[] as hidden? I don't understand the whole logic, so I don't know the impact that can occur when I simply filter in Nodes().

Is there any way to make separate calls to master and slave nodes like redis-go in cluster mode?

rueian commented 3 months ago

Is there any particular reason for initaddress to be saved in conns[] as hidden?

Initial addresses are always saved so that the client can always get topology from them. If initial addresses are not saved, they can be forgotten by the client when topology changes, and then the client is out of your control until you restart it with the same initial addresses.

I don't understand the whole logic, so I don't know the impact that can occur when I simply filter in Nodes().

I believe there is no impact.

Is there any way to make separate calls to master and slave nodes like redis-go in cluster mode?

Could you elaborate more on this?