Core: Add AzAffinityAllNodes Read Strategy #2986
Conversation
Muhammad-awawdi-amazon
added
Feature
glide-core/redis-rs/redis/src/cluster_async/connections_container.rs
Outdated
Show resolved
Hide resolved
| @@ -302,6 +302,103 @@ mod cluster_async { | |||
| ); | |||
| } | |||
|
|
|||
| #[tokio::test] | |||
| async fn test_routing_by_slot_to_replica_with_az_affinity_all_nodes_strategy_to_half_replicas() | |||
There was a problem hiding this comment.
Run the exiting test twice, once for AZAffinity and once for the new strategy
There was a problem hiding this comment.
what do you mean here? this test already have 2 versions for both of the AZAffinity read strategies
1- async fn test_routing_by_slot_to_replica_with_az_affinity_all_nodes_strategy_to_half_replicas() (ReplicasAndPrimary)
2- async fn test_routing_by_slot_to_replica_with_az_affinity_strategy_to_half_replicas() (AzAffinity)
| @@ -392,6 +489,209 @@ mod cluster_async { | |||
| ); | |||
| } | |||
|
|
|||
| #[tokio::test] | |||
| async fn test_routing_by_slot_to_replica_with_az_affinity_all_nodes_strategy_to_all_replicas() { | |||
There was a problem hiding this comment.
same here. use the existing test
There was a problem hiding this comment.
same as above, 2 versions of this test too
| let cluster = | ||
| TestClusterContext::new((replica_num * primaries_num) + primaries_num, replica_num); | ||
| let client_az = "us-east-1a".to_string(); | ||
| let other_az = "us-east-1b".to_string(); |
There was a problem hiding this comment.
you dont need it. it will be empty string anyway, and different from client az
There was a problem hiding this comment.
Technically speaking, yes.
thats make it more clear though, 😄
| .route_command( | ||
| &cmd, | ||
| RoutingInfo::SingleNode(SingleNodeRoutingInfo::SpecificNode(Route::new( | ||
| 10923, // This should target the third primary |
There was a problem hiding this comment.
Why did you not use the same slot as previous tests that mapped to the foo key?
There was a problem hiding this comment.
you're right that would be consistent with other tests
changing it to 12182
| @@ -288,6 +288,64 @@ where | |||
| } | |||
| } | |||
|
|
|||
| /// Returns the node's connection in the same availability zone as `client_az`, | |||
| /// checking replicas first, then primary, and falling back to any available node. | |||
| pub(crate) fn round_robin_read_from_replica_with_az_awareness_all_nodes( | |||
There was a problem hiding this comment.
It would be better to create a helping function and to use the same logic for both functions with flag, this one and the already exits round_robin_read_from_replica_with_az_awareness function. Both functions logic except of step 2, is duplicate
There was a problem hiding this comment.
I think since we already separated most of the tests between them it would be more consistent to have this test separated too. it will be more easier to understand if another user reviews it
Muhammad-awawdi-amazon
force-pushed
the
primary-az-read
branch
from
a36c302 to
5183314
Compare
Signed-off-by: Muhammad Awawdi <[email protected]>
…asAndPrimary Signed-off-by: Muhammad Awawdi <[email protected]>
Signed-off-by: Muhammad Awawdi <[email protected]>
Signed-off-by: Muhammad Awawdi <[email protected]>
Muhammad-awawdi-amazon
force-pushed
the
primary-az-read
branch
from
09378d2 to
ea5954c
Compare
Signed-off-by: Muhammad Awawdi <[email protected]>
Signed-off-by: Muhammad Awawdi <[email protected]>
Issue link
This Pull Request is linked to issue (URL): #2792
Checklist
Before submitting the PR make sure the following are checked: