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

Jump to bottom

Add support for reading from replicas in non-CLUSTER environments #8

Open
jgaskins opened this issue Nov 11, 2021 · 5 comments · May be fixed by #50
Open

Add support for reading from replicas in non-CLUSTER environments #8

jgaskins opened this issue Nov 11, 2021 · 5 comments · May be fixed by #50

Comments

@jgaskins
Copy link
Owner

I think we can handle this by accepting a list of URIs in Client#initialize and assigning them at random in the connection pool block.

Thinking something like this:

    # Keep this method signature for backwards-compatibility
    def self.new(uri : URI)
      max_idle_pool_size = uri.query_params.fetch("max_idle_pool_size", "25").to_i
      new([uri], max_idle_pool_size)
    end

    def initialize(uris : Array(URI) = [URI.parse("redis:///")], max_idle_pool_size : Int = 25)
      @pool = DB::Pool.new(
        max_idle_pool_size: max_idle_pool_size.to_i32,
      ) { Connection.new(uris.sample) }
    end
@jgaskins
Copy link
Owner Author

I can't remember if this is a thing with Redis, but we may also need to adjust to network topology after we connect in case we don't have all the IP addresses or servers are added/removed after we connect, etc. This is common with a lot of clustered resources, but I don't know if Redis provides this information to clients.

@jgaskins
Copy link
Owner Author

Cluster support is added in ca206a7 but it doesn't handle plain replication (single primary shard, many replicas). Redis doesn't call this "clustering" and none of the CLUSTER commands are available in this mode, so I'm not sure what the client abstraction for this particular deployment configuration should be.

And, realistically speaking, do Redis clusters exist in the wild that do simple replication without cluster-mode enabled?

I'm still also unsure that the current abstraction split between Client and Cluster is ideal. The Client maintains a connection pool but the Cluster maintains multiple connection pools. Should the Cluster instead maintain a collection of Clients rather than messing with its own connection pools? 🤔

@jwoertink
Copy link
Contributor

Just wanted to add to this. Our redis env is using cluster mode. stefanwille/crystal-redis#127 There's a docker-compose example in this issue that could be used for some local testing on this.

@jgaskins
Copy link
Owner Author

jgaskins commented Jun 19, 2022 •
edited

@jwoertink Thanks for jumping in here. If I’m reading that issue correctly[1], this shard already supports that particular type of Redis clustering. This issue is still open only to handle the plain-old-replication form of clustering (via REPLICAOF), which Redis confusingly doesn’t call a “cluster”. This shard does support reading from replicas in sharded clusters, though.

If you use this shard on your Redis cluster (using Redis::Cluster instead of Redis::Client), does it work? For example:

cluster = Redis::Cluster.new(REDIS_URL)

pp cluster.lrange(“thing”, “0”, “-1”)

(pardon the smart quotes in that code, I’m writing on an iPad)

[1] The MOVED error is raised when the client talks to the wrong Redis server in the cluster for that key, and is solved by the Redis::Cluster class.

@jwoertink
Copy link
Contributor

oh nice! I'll have to give that a shot and let you know how it goes. That's for the info 🚀

@jgaskins jgaskins changed the title Add support for clusters Add support for reading from replicas in non-CLUSTER environments Sep 20, 2022
@jgaskins jgaskins linked a pull request Feb 1, 2024 that will close this issue
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Add ReplicationClient jgaskins/redis
2 participants
@jwoertink @jgaskins