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.

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

Redis cluster support for do_count_hit #40

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

carlogilmar
Copy link

@carlogilmar carlogilmar commented Jan 31, 2023

Hi! As I mention here: #39 we had problems using this with a Redis Cluster. Looking the documentation looks like the client connector needs to enable a cluster mode to make operations without the MOVED error.

Replication

  1. I created a redis cluster: https://redis.io/docs/management/scaling/#create-and-use-a-redis-cluster
  2. I connected to just one node in the config file.

Possible Solution

  • When you try to execute a SET instruction in a cluster, if the node connected receives the record, you should receive the tuple {:ok, "OK"}.
  • When you try to execute a SET instruction in a cluster but the node connected is not able to store the record, then you should receive a Redix.Error with the moved error message: MOVED 991 127.0.0.1:6001. This means you need to save the record in that specific node.

image

First Approach to design the solution

  1. Validate the MOVED error in the do_count_hit/5 function.
  2. Parse the correct node direction from the redix error.
  3. Create a new connection to that node.
  4. This will happen any time when you are in a cluster, we need to validate the connections to avoid create many connections to the same node. I achieve this registering the node direction as name.

Feedback

I'd like to get feedback on this possible solution, other pending part is add tests for this, but I'm a little bit confusing on that part. What do you think? Maybe there are other solutions.

epinault
epinault previously approved these changes Mar 29, 2024
@epinault
Copy link
Contributor

looks like this need to rebased if you get get a chance

keys_in_cluster =
for conn <- cluster_nodes, reduce: [] do
acc ->
{:ok, keys} = Redix.command(conn, ["KEYS", "*"])

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm new to Redis and just read enough docs to make #56 work but from what I've learned, KEYS command results in a global lock. Is it really OK to be using it here?

@ruslandoga
Copy link

ruslandoga commented Nov 14, 2024

In light of #56, I think this can be implemented as a separate backend. The default one should be the simplest one, I think.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants