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

CROSSSLOT keys issue when using redis cluster #709

Closed
naveen-chidhambaram opened this issue Jul 5, 2023 · 5 comments
Closed

CROSSSLOT keys issue when using redis cluster #709

naveen-chidhambaram opened this issue Jul 5, 2023 · 5 comments
Assignees

Comments

@naveen-chidhambaram
Copy link
Contributor

The change introduced as part of this PR does not work with the Redis Cluster setup. The redis.rename method call raises an error with the message CROSSSLOT Keys in request don't hash to the same slot.

Solution:

  • In Cluster mode, both the keys of RENAME command must be in the same hash slot.
  • Use Hash tags to allot the same slot for the keys.
def persist_list(list_name, list_values)
  if list_values.length > 0
    redis.multi do |multi|
      tmp_list = "#{list_name}_tmp"
      tmp_list_tag = redis_namespace_used? ? "#{Split.redis.namespace}:#{list_name}" : list_name
      tmp_list << "{#{tmp_list_tag}}"
      multi.rpush(tmp_list, list_values)
      multi.rename(tmp_list, list_name)
    end
  end

  list_values
end

private

def redis_namespace_used?
  Redis.const_defined?("Namespace") && Split.redis.is_a?(Redis::Namespace)
end

Can we port the above changes into the gem?

@andrehjr
Copy link
Member

andrehjr commented Jul 5, 2023

Hi, thanks for issue! Yes, please do open a PR 🙏

I'm thinking if there's a way we could avoid referencing Redis::Namespace directly. Or if we could rewrite this method without using rename. That way, we wouldn't need to worry about Hash tags. I'll do some further investigation. it's been a while since I did the original PR.

@naveen-chidhambaram
Copy link
Contributor Author

We considered the below approach of using del and rpush instead of using rename. Below are the reasons for not proceeding with this approach.

  • In redis, when a command fails, all the other commands in the queue are processed and will not stop the processing of commands (reference).
  • Redis does not support rollbacks of transactions as well. That is when a command fails within a redis transaction, the subsequent commands are not executed and redis doesn’t rollback the previously executed commands within that transaction (reference).
def persist_list(list_name, list_values)
  if list_values.length > 0
    redis.multi do |multi|
      multi.del(list_name)
      multi.rpush(list_name, list_values)
    end
  end

  list_values
end

@naveen-chidhambaram
Copy link
Contributor Author

@andrehjr Can you please review the comment and PR #710

@andrehjr
Copy link
Member

I'll look into this some time this week. I had some personal issues that kept me away 😓

@andrehjr
Copy link
Member

Closed by #710

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

No branches or pull requests

2 participants