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

Perform cluster bookeeping tasks _before_ determining retry #300

Merged

Conversation

KJTsanaktsidis
Copy link
Contributor

Just because a block is not going to be retried, does not mean we should not process topology updates & redirections in response to errors, I think.

Before this patch is merged:

  • We catch an exception,
  • We see if we have retry_count remaining,
  • And if so, update cluster topology,
  • And then retry

After this patch is merged:

  • We catch an exception,
  • We update cluster topology (e.g. processing MOVED).
  • If we have retry_count remaining,
  • Then we retry

This is important for cluster transaction support because we will always do user-provided transaction blocks with retry_count: 0 (we don't know if the provided block is safe to call again); but, we need to make sure we still update the topology so that if the user retries themselves, it will actually work.

Just because a block is not going to be retried, does not mean we should
not process topology updates & redirections in response to errors, I
think.
@KJTsanaktsidis KJTsanaktsidis force-pushed the ktsanaktsidis/cluster_bookkeeping branch from 21f643e to f92d39d Compare December 12, 2023 02:33
@supercaracal supercaracal self-requested a review December 12, 2023 06:45
@supercaracal
Copy link
Member

Linter error is not related this pull request. We can ignore them at this time.

@supercaracal supercaracal merged commit e52cf5d into redis-rb:master Dec 12, 2023
24 of 25 checks passed
@supercaracal
Copy link
Member

Thank you!

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.

2 participants