StackExchange / StackExchange.Redis

General purpose redis client
https://stackexchange.github.io/StackExchange.Redis/
Other
5.85k stars 1.5k forks source link

Support Alibaba pseudo-cluster configurations #2646

Closed mgravell closed 4 months ago

mgravell commented 5 months ago

1: don't treat trivial clusters as clusters - Alibaba uses this config 2: report synchronous failures immidiately and accurately

fix #2642

For context, Alibaba reports from cluster nodes as 2 nodes:

aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa {proxy endpoint} myself,master - 0 0 0 connected 0-16383
bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb {proxy endpoint} slave aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa 0 0 0 connected

which confuses things somewhat; this isn't a valid/normal cluster configuration, so: ignore it!

Other relevant metadata:

from config:

databases=256

from info:

# Cluster
cluster_enabled:1
databases:256
nodecount:2
NickCraver commented 5 months ago

How does this affect single node clusters? For example, even in Azure hosting people start with a single cluster node in a lot of cases which makes scaling to n easier later...but they don't support SELECT because they are cluster. I worry about the assumption of 2+ here being needed.

mgravell commented 5 months ago

Untested and it is a good question, but: if the worst side effect here is that we get a server-side "nope" rather than a client-side "nope" in genuine single-server clusters: I'm comfortable with that...?

NickCraver commented 5 months ago

I used SELECT as an example, but I think this had many more side-effects such as not detecting when the cluster changes and such. If for example we were viewing this in Opserver…it'd be wrong. Any code basing off cluster or not on top of us has the same downstream fun. Lots of potential there.

I'd vote we should not make such an assumption based on node count - likely better for the server-side to not represent itself as a cluster here.

mgravell commented 4 months ago

@NickCraver thanks for keeping me honest; refactored to use the "databases" entry as reported from config; this data is also available under info cluster, but I don't propose to add an extra info call unless this doesn't work reliably

yangbodong22011 commented 4 months ago

@mgravell Thank you for your quick fix. We have verified the 2.7.20 version of MyGet and it works very well. When do you expect to release it to NuGet? After that, we will push users to upgrade.

mgravell commented 4 months ago

Thanks for confirming. I'll discuss release in our weekly sync tomorrow (20th) - assuming there's no known blockers, I expect we'll release to NuGet either end-of-day 20th, or very shortly after