Added management connections to be used with the periodic checks #56

barshaul · 2023-10-03T16:58:50Z

This PR adds another connection (called 'management connection') for each cluster node so they could be used in the periodic checks, if the periodic checks is enabled.

Rebased over #64 , #90

Benchmark results:

test	num_of_tasks	data_size	Average of tps	get non existing average latency	get existing average latency	set average latency	TPS diff
python_baseline	1	100	939	0.001	0.001	0.001
python_baseline		4000	921	0.001	0.001	0.001
python_baseline	10	100	6556	0.002	0.002	0.002
python_baseline		4000	6321	0.002	0.002	0.002
python_baseline	100	100	26655	0.004	0.004	0.004
python_baseline		4000	22485	0.004	0.004	0.004
python_baseline	1000	100	28057	0.036	0.035	0.036
python_baseline		4000	24573	0.041	0.042	0.041
python_mange_conn_120_sec	1	100	937	0.001	0.001	0.001	99.83%
python_mange_conn_120_sec		4000	937	0.001	0.001	0.001	101.78%
python_mange_conn_120_sec	10	100	6491	0.002	0.002	0.002	99.01%
python_mange_conn_120_sec		4000	6400	0.002	0.002	0.002	101.26%
python_mange_conn_120_sec	100	100	25537	0.004	0.004	0.004	95.81%
python_mange_conn_120_sec		4000	22479	0.004	0.004	0.004	99.97%
python_mange_conn_120_sec	1000	100	26967	0.037	0.038	0.037	96.12%
python_mange_conn_120_sec		4000	25546	0.038	0.040	0.037	103.96%
python_mang_conn_60_sec	1	100	951	0.001	0.001	0.001	101.30%
python_mang_conn_60_sec		4000	927	0.001	0.001	0.001	100.60%
python_mang_conn_60_sec	10	100	6374	0.002	0.002	0.002	97.22%
python_mang_conn_60_sec		4000	6437	0.002	0.002	0.002	101.84%
python_mang_conn_60_sec	100	100	26341	0.004	0.004	0.004	98.82%
python_mang_conn_60_sec		4000	22788	0.004	0.004	0.004	101.35%
python_mang_conn_60_sec	1000	100	26090	0.038	0.036	0.039	92.99%
python_mang_conn_60_sec		4000	25010	0.040	0.040	0.040	101.78%
Average TPS diff							99.60%

nihohit

initial comments.

This PR is too large to review effectively. Please remove unnecessary changes, and if possible split to PRs, if not possible split to commits.

redis/src/cluster_async/connections_container.rs

nihohit · 2023-10-05T10:56:45Z

redis/src/cluster_async/connections_container.rs

@@ -11,18 +11,40 @@ type IdentifierType = ArcStr;

 #[derive(Clone, Eq, PartialEq, Debug)]
 pub(crate) struct ClusterNode<Connection> {
-    pub connection: Connection,
-    pub ip: Option<IpAddr>,
+    user_connection: Connection,


why did you decide to hide all of the fields?

just so if we'll have a connection pool in the future for user/management connections we'll have less code change, so I kept the same line of get functions for IP too. do you prefer we'll directly access the variables?

I don't see how this reduces the code change in such a situation.

do you prefer we'll directly access the variables?

Yes, empty getters are usually avoided, unless there's an intention for encapsulation.

I don't see how this reduces the code change in such a situation.

now:

fn get_connection { self.connection }

later

fn get_connection { if pool.empty() {} else { pool.get-connection() }

or something like that.
but I can remove it for now

redis/src/cluster_async/mod.rs

nihohit · 2023-10-05T12:16:18Z

redis/src/cluster_async/mod.rs

-        let requested_nodes = read_guard.random_connections(num_of_nodes_to_query);
+        let requested_nodes = read_guard.random_connections(
+            num_of_nodes_to_query,
+            inner.cluster_params.management_connections_enabled(),


so this means that refresh_slots ALWAYS uses management connections, if they are enabled? even if it wasn't triggered by periodic check?

yes, do you see any downside with it?
It solves the issue of blocking/pubsub connections

no, but since you added no documentation or explanations to the PR, I'm trying to understand all of the changes and the reasons behind them.

I'll need to think about this, and understand what's the ramifications.

nihohit · 2023-10-05T12:18:57Z

redis/src/cluster_async/mod.rs

-    let (mut conn, ip) = C::connect(info, socket_addr)
-        .timeout(connection_timeout)
-        .await??;
+    let connection_timeout: futures_time::time::Duration = params.connection_timeout.into();


why is the type definition needed?

without it - "..type inside async fn body must be known in this context"

nihohit · 2023-10-26T15:31:47Z

why is management_connections_enabled configurable? what will be the default that we offer the users, and when would the users prefer to change the configuration?

nihohit · 2023-10-26T15:32:46Z

and re: the benchmarks - the 100 bytes results are pretty bad. Is that the average of multiple runs?
Also, does the benchmark result change if you use this branch, but change management_connections_enabled?

barshaul · 2023-10-28T13:03:16Z

@nihohit Updated benchmark results

nihohit · 2023-10-29T07:05:08Z

Those are averages of multiple runs? I'm trying to understand how robust is the 7% difference in one scenario.

It's generally preferable to benchmark with Node, since the higher TPS mean that there's less overhead to mask/compensate for small effects, and the benchmarks conclude faster, which allows first more iterations.

nihohit · 2023-11-02T10:27:01Z

what's the 60_sec / 120_sec difference in the benchmarks?

redis/tests/test_cluster_async.rs

redis/src/cluster_async/mod.rs

barshaul · 2023-11-06T08:43:43Z

@nihohit Node benchmark results:

language	num_of_tasks	data_size	Average of tps	Average of get_non_existing_average_latency	Average of get_existing_average_latency	Average of set_average_latency	TPS diff
node_periodic_check_enabled_60_sec_managed_conn	1	100	1009	0.989	0.990	0.996	101.77%
		4000	971	1.005	1.031	1.044	99.79%
	10	100	6168	1.621	1.621	1.625	101.48%
		4000	6083	1.628	1.652	1.639	103.02%
	100	100	55365	1.805	1.806	1.807	98.72%
		4000	48900	2.042	2.057	2.011	98.91%
	1000	100	119312	8.379	8.381	8.382	101.27%
		4000	69673	14.352	14.366	14.330	100.95%
node_periodic_check_enabled_60_secs_non_managed_conn	1	100	1001	0.995	0.997	1.008	100.18%
		4000	975	1.002	1.027	1.041	98.43%
	10	100	5983	1.669	1.670	1.675	102.44%
		4000	6049	1.638	1.660	1.646	99.75%
	100	100	55941	1.787	1.788	1.787	98.17%
		4000	48530	2.057	2.074	2.026	100.64%
	1000	100	118565	8.434	8.434	8.436	99.49%
		4000	69323	14.430	14.444	14.410	101.11%
node_periodic_check_disabled_management_branch	1	100	1003	0.995	0.995	1.006	100.33%
		4000	976	1.001	1.027	1.036	99.28%
	10	100	6034	1.657	1.658	1.660	103.69%
		4000	6123	1.617	1.642	1.625	99.12%
	100	100	55591	1.800	1.802	1.802	99.80%
		4000	49339	2.025	2.039	1.992	99.61%
	1000	100	117357	8.520	8.522	8.522	100.01%
		4000	69690	14.349	14.359	14.323
node_periodic_check_disabled_main_branch (baseline)	1	100	992	1.005	1.007	1.015
		4000	973	1.004	1.028	1.043
	10	100	6078	1.643	1.645	1.649
		4000	5905	1.676	1.699	1.688
	100	100	56082	1.782	1.783	1.785
		4000	49437	2.017	2.035	1.990
	1000	100	117814	8.487	8.488	8.490
		4000	69680	14.349	14.362	14.326

barshaul · 2023-11-06T08:49:10Z

why is management_connections_enabled configurable? what will be the default that we offer the users, and when would the users prefer to change the configuration?

@nihohit
We should separate between the use of managed connections to the periodic checks (i'll refactor it).
For periodic checks, ATM when we have issues with how we compare CLUSTER SLOTS results, we should keep it disabled by default, and enable it once the fix is done.
For management connections, we should use them by default and make it non-configurable.
WDYT?

barshaul · 2023-11-12T15:10:36Z

@nihohit round

barshaul · 2023-12-18T10:48:36Z

@shachlanAmazon round

barshaul · 2023-12-21T17:24:54Z

@shachlanAmazon Lets have a code walkthrough on Sunday and i'll go through the latest changes I did. Benchmarks are in progress

nihohit

The main issue I have is that you've added a lot of logic & branching, and the vast majority of it isn't tested. I'm not sure that it's possible to test this using the existing cluster testing infra structure - maybe the connection management logic should be spun off to a separate file / module, and tested separately?

redis/src/cluster_async/mod.rs

barshaul · 2023-12-26T09:44:26Z

@shachlanAmazon Benchmark results in rust show decrease in up to ~5%. I'm not sure why these changes affect the happy path, I also tested mainline with periodic checks without the addition of management connections and I haven't seen this degradation. I'll add benchmark results of this branch without periodic checks and mainline with periodic checks, and node benchmark results.
Anyway, we decided we won't put too much effort at the moment when the degradation is up to 5%, but if you see something that pops out or if you want to help with quick perf to see if something pops out, let me know.

language	client	num_of_tasks	data_size	Average of tps	Average of get_non_existing_average_latency	Average of get_existing_average_latency	Average of set_average_latency	TPS diff
rust	mainline_no_periodic	1	100	5038	0.198	0.199	0.200
rust			4000	4789	0.185	0.214	0.211
rust		10	100	33667	0.299	0.299	0.300
rust			4000	30456	0.317	0.331	0.329
rust		100	100	210654	0.474	0.475	0.475
rust			4000	104847	0.949	0.957	0.946
rust		1000	100	277776	3.598	3.599	3.599
rust			4000	145832	6.854	6.855	6.859
rust	management_conn_60_sec	1	100	5057	0.196	0.199	0.199	100.37%
rust			4000	4758	0.189	0.213	0.218	99.35%
rust		10	100	31663	0.318	0.319	0.319	94.05%
rust			4000	28841	0.335	0.350	0.347	94.70%
rust		100	100	205164	0.486	0.487	0.488	97.39%
rust			4000	107063	0.931	0.937	0.928	102.11%
rust		1000	100	265918	3.759	3.759	3.760	95.73%
rust			4000	141053	7.086	7.087	7.096	96.72%

… to the user connection if needed

shachlanAmazon

round.
absolutely great work on the tests. well done!

redis/src/cluster_async/connections_logic.rs

shachlanAmazon · 2024-01-03T08:30:37Z

redis/src/cluster_async/connections_logic.rs

+    AsyncClusterNode::new(user_conn, management_conn, ip)
+}
+
+pub(crate) async fn connect_and_check_all_connections<C>(


all_connections -> both_connections
Initially I thought this is about running all connections in a cluster.

changed to connect_and_check_all_node_connections

redis/tests/test_async_cluster_connections_logic.rs

shachlanAmazon · 2024-01-03T10:17:46Z

redis/tests/test_async_cluster_connections_logic.rs

+    conn_utils.returned_ip_type = ConnectionIPReturnType::Specified(ip);
+    drop(write_lock);
+    runtime.block_on(async {
+        let node: RedisResult<AsyncClusterNode<MockConnection>> = connect_and_check(


minor: use type alias instead of repeating this long declaration throughout.

shachlanAmazon · 2024-01-03T10:35:29Z

redis/tests/test_async_cluster_connections_logic.rs

+            None,
+        )
+        .await;
+        assert!(node.is_ok() && node.unwrap().management_connection.is_none());


throughout the tests - can we use MockConnection::id (or some other tool) to know which connection was chosen?

shachlanAmazon · 2024-01-03T11:07:41Z

redis/tests/test_async_cluster_connections_logic.rs

+        .get_mut(name)
+        .unwrap_or_else(|| panic!("MockConnectionUtils for `{name}` were not installed"));
+    conn_utils.return_connection_err = ShouldReturnConnectionError::Yes;
+    let conn_type: RefreshConnectionType = RefreshConnectionType::AllConnections;


minor: why do you need the type declaration?

shachlanAmazon · 2024-01-03T11:09:14Z

redis/tests/test_async_cluster_connections_logic.rs

+        .await;
+        assert!(node.is_err());
+        let err = node.unwrap_err();
+        assert!(err


minor: please also assert on err.kind(), throughout

shachlanAmazon · 2024-02-12T19:31:17Z

@barshaul do you want to keep this open?

barshaul force-pushed the management_conn branch 3 times, most recently from f8c92a1 to 2aa84ce Compare October 4, 2023 13:59

barshaul marked this pull request as ready for review October 4, 2023 13:59

barshaul requested a review from nihohit October 4, 2023 14:09

barshaul changed the title ~~WIP: Added management connections to be used with the periodic checks~~ Added management connections to be used with the periodic checks Oct 4, 2023

barshaul force-pushed the management_conn branch from 2aa84ce to ad7dc11 Compare October 4, 2023 14:14

nihohit reviewed Oct 5, 2023

View reviewed changes

barshaul force-pushed the management_conn branch from ad7dc11 to 990425f Compare October 5, 2023 14:51

barshaul force-pushed the management_conn branch 5 times, most recently from 4fb8852 to 33f18da Compare October 26, 2023 15:17

barshaul force-pushed the management_conn branch from 33f18da to 3cea586 Compare October 26, 2023 15:32

nihohit reviewed Nov 2, 2023

View reviewed changes

barshaul force-pushed the management_conn branch 6 times, most recently from 951617c to 5a3e7b0 Compare November 12, 2023 14:59

barshaul force-pushed the management_conn branch 2 times, most recently from c4d680e to 0fc47c5 Compare December 18, 2023 10:40

barshaul force-pushed the management_conn branch 2 times, most recently from fe1237c to 42dffe6 Compare December 21, 2023 16:57

barshaul added 4 commits December 25, 2023 15:54

Added connection management to the connections container

d3830f6

Added management connections to be used with the periodic checks

5987e06

Added management connections to be used with the periodic checks

b1f0a03

tmp commit

d29dd51

barshaul force-pushed the management_conn branch from 42dffe6 to d626d19 Compare December 25, 2023 15:58

nihohit reviewed Dec 26, 2023

View reviewed changes

Changed the managment connection to only be preferred but to fallback…

877e5f2

… to the user connection if needed

barshaul force-pushed the management_conn branch 5 times, most recently from 7f80d5b to d31c834 Compare December 28, 2023 14:15

barshaul mentioned this pull request Dec 28, 2023

Added tests for the management connection #88

Closed

barshaul force-pushed the management_conn branch 6 times, most recently from 65f67ce to edec2e8 Compare January 2, 2024 10:42

shachlanAmazon reviewed Jan 3, 2024

View reviewed changes

barshaul force-pushed the management_conn branch from f0adab3 to 877e5f2 Compare January 15, 2024 09:08

barshaul closed this Feb 25, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added management connections to be used with the periodic checks #56

Added management connections to be used with the periodic checks #56

barshaul commented Oct 3, 2023 •

edited

Loading

nihohit left a comment

nihohit Oct 5, 2023

barshaul Oct 5, 2023

nihohit Oct 5, 2023

barshaul Oct 9, 2023 •

edited

Loading

nihohit Oct 5, 2023

barshaul Oct 5, 2023

nihohit Oct 5, 2023

nihohit Oct 5, 2023

nihohit Oct 5, 2023

barshaul Oct 5, 2023

nihohit commented Oct 26, 2023

nihohit commented Oct 26, 2023

barshaul commented Oct 28, 2023

nihohit commented Oct 29, 2023

nihohit commented Nov 2, 2023

barshaul commented Nov 6, 2023 •

edited

Loading

barshaul commented Nov 6, 2023 •

edited

Loading

barshaul commented Nov 12, 2023

barshaul commented Dec 18, 2023

barshaul commented Dec 21, 2023

nihohit left a comment

barshaul commented Dec 26, 2023

shachlanAmazon left a comment

shachlanAmazon Jan 3, 2024

barshaul Jan 3, 2024

shachlanAmazon Jan 3, 2024

shachlanAmazon Jan 3, 2024

shachlanAmazon Jan 3, 2024

shachlanAmazon Jan 3, 2024

shachlanAmazon commented Feb 12, 2024

Added management connections to be used with the periodic checks #56

Added management connections to be used with the periodic checks #56

Conversation

barshaul commented Oct 3, 2023 • edited Loading

nihohit left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

barshaul Oct 9, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nihohit commented Oct 26, 2023

nihohit commented Oct 26, 2023

barshaul commented Oct 28, 2023

nihohit commented Oct 29, 2023

nihohit commented Nov 2, 2023

barshaul commented Nov 6, 2023 • edited Loading

barshaul commented Nov 6, 2023 • edited Loading

barshaul commented Nov 12, 2023

barshaul commented Dec 18, 2023

barshaul commented Dec 21, 2023

nihohit left a comment

Choose a reason for hiding this comment

barshaul commented Dec 26, 2023

shachlanAmazon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shachlanAmazon commented Feb 12, 2024

barshaul commented Oct 3, 2023 •

edited

Loading

barshaul Oct 9, 2023 •

edited

Loading

barshaul commented Nov 6, 2023 •

edited

Loading

barshaul commented Nov 6, 2023 •

edited

Loading