Added logic to update the slot map based on MOVED errors #186
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…hard between shard nodes and slot map values
barshaul
force-pushed
the
update_on_moves
branch
4 times, most recently
from
f41533b
to
b0a3e5c
Compare
barshaul
force-pushed
the
update_on_moves
branch
2 times, most recently
from
a374c59
to
e17a802
Compare
barshaul
force-pushed
the
update_on_moves
branch
from
e17a802
to
7b675bf
Compare
barshaul
changed the title
eifrah-aws
requested changes
Sep 11, 2024
redis/src/cluster_async/mod.rs
Outdated
| // If it fails, proceed by retrying the request with the redirected node, | ||
| // and allow the slot refresh task to correct the slot map. | ||
| warn!( | ||
| "Failed to update the slot map based on the received MOVED error.\n |
There was a problem hiding this comment.
- Avoid using
\nin the log as it create line without its prefix. Please use 2 xwarn!calls - If it is OK to fail, why use
warn!? (this is a good place to use telemetry call - when its available)
redis/src/cluster_async/mod.rs
Outdated
|
|
||
| if curr_shard_addrs_read.replicas().contains(&new_primary) { | ||
| // Scenario 2: Failover - The new primary is a replica within the same shard | ||
| drop(curr_shard_addrs_read); |
There was a problem hiding this comment.
Quesiton: we are dropping and then re-acquiring it, meaning: we are losing atomicity - does this may yield a problem for us?.
If it does, it better to obtain write lock to begin with
There was a problem hiding this comment.
Yes you're right I found some potential issues. I’ve updated it to acquire the write lock at the start
| } | ||
|
|
||
| // Scenario 5: New Node - The new primary is not present in the current slots map, add it as a primary of a new shard. | ||
| drop(nodes_iter); |
There was a problem hiding this comment.
Why the explicit drop here? won't it be dropped when leaving the function?
There was a problem hiding this comment.
I have to drop it bc it borrows connection container as immutable and we need to do some mutable borrowing next line
barshaul
force-pushed
the
update_on_moves
branch
from
9ce7c58
to
58564f6
Compare
eifrah-aws
approved these changes
Sep 12, 2024
barshaul
merged commit c48287a
into
amazon-contributing:update_slotmap_moved
10 checks passed
barshaul
added a commit
that referenced
this pull request
Sep 12, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Added logic to update the slot map based on MOVED errors. Now, when a MOVED error is received, the client will push a new future before retrying the request to attempt to update the slot map based on the new primary node, while still spawning a background task to refresh slots. This provides an optimization to ensure quicker rerouting of requests without waiting for the background slot refresh which can be skipped due to the rate limiter.
The updated logic handles several scenarios:
If the slot map update fails, the request is retried with the moved redirect node, and the background slot refresh task will correct the map asynchronously.