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

Is renewSlotCache necessary when received MOVED #3821

Open
YorigamiShion opened this issue Apr 20, 2024 · 3 comments
Open

Is renewSlotCache necessary when received MOVED #3821

YorigamiShion opened this issue Apr 20, 2024 · 3 comments
Labels

Comments

@YorigamiShion
Copy link

Expected behavior

Just set JedisClusterInfoCache.slots[currentSlot] = targetConnection might be more efficient.

Actual behavior

When received MOVED, ClusterCommandExecutor will call ClusterConnectionProvider#renewSlotCache and then clear ``JedisClusterInfoCache.slots` and reassign them in a write lock.

Steps to reproduce:

Any action for redis server responses MOVED

  1. Send write command to slave node (e.g. failover)
  2. Send command to the node who does not owns the slot (usually after slot migration completed)
@sazzad16
Copy link
Collaborator

We cannot fully adopt your suggestion because - i) Current implementation is based on Redis' proposed algorithm. ii) Usually MOVED happens to a large number of slots once; so renewing all is the faster way to update.

But I understand that for some users it might make more sense to renew slots one by one per MOVED. So we'll consider a PR that'll allow users to set a flag/config/param to update one slot (in slot cache) per MOVED instead of updating all slots.

@YorigamiShion
Copy link
Author

We cannot fully adopt your suggestion because - i) Current implementation is based on Redis' proposed algorithm. ii) Usually MOVED happens to a large number of slots once; so renewing all is the faster way to update.

But I understand that for some users it might make more sense to renew slots one by one per MOVED. So we'll consider a PR that'll allow users to set a flag/config/param to update one slot (in slot cache) per MOVED instead of updating all slots.

How about compare with previos cluster topology?
If MOVED to a new master, just change slot owner without renewSlotCache.
If MOVED to slot owner's slave, maybe failover happened, set all slots of the shard to new master or just renewSlotCache which is more safe.
Otherwise, call renewSlotCache.

@sazzad16
Copy link
Collaborator

If MOVED to slot owner's slave, maybe failover happened, ...

"Maybe".
Maybe not.
What if the cluster is reconfigured where a replica (slave) is promoted to master with half of the slots?
Should a client library "guess" or fetch the information and use it?
To me, it's undoubtedly the later option.

If ..., just change slot owner without renewSlotCache.

Almost as I said in my previous comment.

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

No branches or pull requests

2 participants