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

fix: cache auto retry on MOVED / ASK / ... #701

Merged
merged 7 commits into from
Dec 17, 2024

Conversation

wyxloading
Copy link
Contributor

@wyxloading wyxloading commented Dec 16, 2024

try fix #700

DoCache, DoMultiCache end out with one or some redis transaction like:
MULTI, PTTL {key}, GET {key}, EXEC

When we scale a redis-cluster, PTTL {key} may return a MOVED / ASK error because slot have mark migrate / migrating / migrated to another node, then the transaction may fail.

We hope driver can handle this case automaticlly by just retry the transaction on node which error said.

EDIT:
We tested in our real test environment and it works.

@wyxloading
Copy link
Contributor Author

BTW, happened to find out testcase TestClusterClient_SendToOnlyPrimaryNodeWhenPrimaryNodeSelected Do with no slot may fail sometimes. Seems like driver will pick any conn to do initSlot commands, but testcase assume it only use primary node conn

@rueian
Copy link
Collaborator

rueian commented Dec 16, 2024

BTW, happened to find out testcase TestClusterClient_SendToOnlyPrimaryNodeWhenPrimaryNodeSelected Do with no slot may fail sometimes. Seems like driver will pick any conn to do initSlot commands, but testcase assume it only use primary node conn

Thank you for catching that. We can just delete all the Do with no slot test cases because we don't guarantee using primaries for those commands anymore. In the future, nodes may have mixed roles of primaries and replicas.

cluster.go Outdated Show resolved Hide resolved
cluster.go Outdated Show resolved Hide resolved
pipe.go Outdated Show resolved Hide resolved
@wyxloading
Copy link
Contributor Author

@rueian all review discussions resolved, please check this out.

cluster.go Show resolved Hide resolved
@rueian rueian merged commit 8aa9298 into redis:main Dec 17, 2024
30 checks passed
@rueian
Copy link
Collaborator

rueian commented Dec 17, 2024

Thank you @wyxloading!

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.

Retry DoXCache during redis cluster scaling
2 participants