[2/5] cluster: refactor param passing & add params to the builder #670

utkarshgupta137 · 2022-08-23T16:59:36Z

Currently, all the params in the ClusterConnection have individual fields in the struct, which makes it challenging to add new parameters to the client & also forces us to pass 3-4 params over & over (user/password/tls_insecure etc.). Also, there is no way to provide default values for the parameters that can be set on the connection level.

With this PR, I've refactored the cluster connection to store & pass all the parameters using the ClusterParams struct added in the previous PR & also added functions to the builder to set default values for each.

In addition, I've made the following changes:

Simplified logic around TlsMode
- Currently, we check if the connection is a Tls connection or not as well as the value of the insecure param each time we get a connection. The value of the insecure param is also added to the node string, which is used as a key to get connections.
- Instead, it is safe to assume that the connection type (Tls or not) & insecure flag is the same for all the nodes in the cluster. Thus, we can just parse the insecure flag once within the build() function in the cluster client builder & only use it when we need to create the connection
- This also simplifies creating the node string from the ConnectionAddr
Converted most functions in the impl to methods, i.e. used self instead of passing all the required params
Added configuration for connect_timeout & retries
Moved the slots response parsing logic to the get_slots function
- This is the only change not related to the cluster client, but it will make the next PR easier to review

Depends on #669.

redis/src/cluster_client.rs

utkarshgupta137 · 2022-08-31T13:14:20Z

@jaymell @djc Ready for review. I've kept all the code moving around in a single commit.

redis/src/cluster_client.rs

jaymell · 2022-09-07T05:59:25Z

I want to review the major refactors again one last time but overall I think this looks great and simplifies a lot of the existing code.

djc

I did a first round of review, which has made me grumpy. Many of these commits have too much going on at the same time to be easy to review, requiring me to scroll up and down to keep track of what's going on where. In some cases you have removed comments only to re-add them in a later commit, requiring me to keep track of those changes. Making commits one logical change at a time means that shouldn't happen.

Don't get me wrong, I like the direction here and I think the code is better off at the end, but as a reviewer getting there is much more painful than it should be if these commits were properly factored and better used the commit messages to explain the purpose of changes (although I noticed some attempts at doing this, which I appreciated).

redis/src/cluster.rs

redis/src/macros.rs

djc · 2022-09-08T10:47:39Z

redis/src/cluster.rs

-            format!("rediss://{}/#insecure", host_port)
-        }
-        Some(TlsMode::Secure) => format!("rediss://{}", host_port),
+fn get_connection_addr(node: (String, u16), tls_insecure: Option<bool>) -> ConnectionAddr {


Please ditch the get_ prefix (see the API guidelines).

What name do you suggest instead?

Just use connection_addr()? But alternatively, maybe this should be a constructor on ConnectionAddr?

The get_ prefix is used in too many places in the existing code base. I think it would be best to keep the name the same.
I've made it a function of ClusterParams as it doesn't make sense to create a constructor for ConnectionAddr which requires TlsMode which is only used by cluster.

redis/src/cluster_client.rs

djc · 2022-09-08T10:56:13Z

redis/src/cluster_client.rs

@@ -27,7 +32,10 @@ impl ClusterClientBuilder {
                .into_iter()
                .map(|x| x.into_connection_info())
                .collect(),
-            cluster_params: ClusterParams::default(),
+            cluster_params: ClusterParams {
+                auto_reconnect: true,


This seems like an unrelated change?

No, the default value of auto_reconnect should be true, not false.

So it's a new option?

No, but it is moved from ClusterConnection to ClusterParams.

djc · 2022-09-08T11:01:13Z

redis/src/cluster_client.rs

@@ -145,7 +145,10 @@ impl ClusterClientBuilder {
    /// Sets the default timeout for all new connections (default is None).
    ///
    /// If the value is `None`, then `get_connection` call may block indefinitely.
-    /// Passing Some(Duration::ZERO) to this method will result in an error.
+    ///


These changes should be folded back into the commits that introduced them.

djc · 2022-09-08T11:01:36Z

redis/src/cluster_client.rs

@@ -210,11 +210,12 @@ impl ClusterClientBuilder {
        self
    }

-    /// Sets the default auto reconnect parameter for all new connections (default is true).
+    /// Disables auto reconnect for all new connections (default is enabled).


Please fold this into the preceding commit.

djc · 2022-09-08T11:02:26Z

redis/src/cluster.rs

-
-            [slot_data.master().to_string(), replica]
-        })?;
+        *slots = self.create_new_slots()?;


Moving this code should be a separate commit from the other changes, to the extent possible.

djc · 2022-09-08T11:03:07Z

redis/src/cluster.rs

-                        .map(|slot_data| (slot_data.end(), get_addr(slot_data)))
-                        .collect(),
-                );
+            if let Ok(Value::Bulk(response)) = cmd.query(conn) {


Can this be a separate commit, too?

- move methods in order: constructor, pub fn, pub(crate) fn, fn - move connect fn inside impl & convert it to a method later on - move trait impl next to main impl - move map_cmds_to_nodes with related functions - remove duplicate check_connection

- use plain host:port node naming, even for TLS as the tls mode/insecure flag will always be the same for all connections

- create new Connection directly instead of creating a Client first - convert to methods & use required params from self instead of passing - set connections within create_initial_connections & call refresh_slots

… cluster related params - move get_connection_info & get_connection_addr to ClusterParams - move validate_duration logic to ClusterParams - remove unnecessary RefCell from params - this is to simplify passing multiple params to & inside the impl

jaymell · 2022-09-21T15:51:53Z

I've gone back through the PR, and I'm about ready to approve but have one more ask: Since you've done the noble work of making the slot parsing logic into a pure function, can we add some unit tests of that function? I think it would be very valuable to add test coverage there.

utkarshgupta137 · 2022-09-21T15:56:53Z

I can add them, but I would like to get this PR merged first so you can review the next PR & the tests-related PR in parallel.

jaymell · 2022-09-21T19:01:11Z

I would feel a lot better about merging this if the code had unit tests.

djc · 2022-09-22T20:00:12Z

redis/src/cluster.rs

@@ -64,6 +64,12 @@ use crate::cluster_routing::{Routable, RoutingInfo, Slot, SLOT_SIZE};

 type SlotMap = BTreeMap<u16, [String; 2]>;

+#[derive(Clone, Copy)]


This should stay above its impl block. I guess I missed this the first time around?

We've removed impl block for TlsMode.

djc · 2022-09-22T20:06:33Z

redis/src/cluster.rs

-        Some(port) => format!("{}:{}", host, port),
-        None => host.to_string(),
-    };
+fn get_connection_addr(node: (String, u16), tls_mode: Option<TlsMode>) -> ConnectionAddr {


If this commit is about TlsMode, don't change the name and signature for this function as part of this commit.

djc · 2022-09-22T20:17:45Z

redis/src/cluster.rs

@@ -775,6 +737,17 @@ fn get_slots(connection: &mut Connection, tls_insecure: Option<bool>) -> RedisRe
    Ok(result)
 }

+fn get_connection_info(node: &str, cluster_params: &ClusterParams) -> ConnectionInfo {


Okay, I'm sorry but this isn't working for me. Here you just add a new function that gets a &str and starts to parse it with two unwrap() calls without any clarity about how the invariants are upheld that these are actually Some. I think the changes you are making are probably for the better, but you have so much stuff in flight at the same time that I'm unable to keep all the context around to do a review with the level of care that I'm used to.

At this point I don't want to review these many-commit PRs from you anymore. I would like one PR at a time, one commit per PR, and if the commit message has a bullet list in it you should split it up more.

djc · 2022-09-26T08:01:11Z

Going to close this for now. Would be great if you can open a small PR to move forward on the refactoring.

utkarshgupta137 force-pushed the cluster_client_refactor branch from 493f776 to ad326cb Compare August 23, 2022 19:01

utkarshgupta137 mentioned this pull request Aug 23, 2022

[3/5] cluster: refactor/optimize for Async Cluster #653

Open

jaymell reviewed Aug 25, 2022

View reviewed changes

redis/src/cluster_client.rs Outdated Show resolved Hide resolved

utkarshgupta137 force-pushed the cluster_client_refactor branch 2 times, most recently from e98553b to 44af956 Compare August 31, 2022 13:12

utkarshgupta137 force-pushed the cluster_client_refactor branch 2 times, most recently from 0c731a5 to f9c3444 Compare September 5, 2022 22:48

jaymell reviewed Sep 7, 2022

View reviewed changes

redis/src/cluster_client.rs Show resolved Hide resolved

djc reviewed Sep 8, 2022

View reviewed changes

utkarshgupta137 added 15 commits September 8, 2022 17:21

cluster: move code

70037d3

- move methods in order: constructor, pub fn, pub(crate) fn, fn - move connect fn inside impl & convert it to a method later on - move trait impl next to main impl - move map_cmds_to_nodes with related functions - remove duplicate check_connection

cluster: reorganise imports by module

e50a2aa

cluster_client: minor refactor & doc changes

197d2f5

cluster: simplify TlsMode logic and add tls_insecure param

559672c

- use plain host:port node naming, even for TLS as the tls mode/insecure flag will always be the same for all connections

cluster: refactor connect & create_initial_connections

f9c27ae

- create new Connection directly instead of creating a Client first - convert to methods & use required params from self instead of passing - set connections within create_initial_connections & call refresh_slots

cluster: add read/write timeout params to builder

7ecf9f4

cluster: add auto_reconnect param to builder

6dcc7e0

cluster_client: remove unncessary bool value from auto_reconnect

d96f92b

cluster: add configurable connect timeout

b686856

cluster: add configurable retries

f9ce44a

cluster_conn: add set_read_from_replicas

1e1eef8

cluster: move CLUSTER SLOTS query from get_slots to create_new_slots

442c821

cluster: move slot checks from create_new_slots to parse_slots_response

c407c80

cluster: move SlotMap logic from refresh_slots to parse_slots_response

4fe97ec

utkarshgupta137 force-pushed the cluster_client_refactor branch from f9c3444 to 4fe97ec Compare September 8, 2022 12:45

djc reviewed Sep 22, 2022

View reviewed changes

djc closed this Sep 26, 2022

0xWOF mentioned this pull request Jan 10, 2023

cluster: simplify TlsMode logic, add param to builder, & refactor create_initial_connections/connect #690

Open

0xWOF mentioned this pull request Feb 7, 2023

[2/5] [8] cluster: keep all params in ClusterParams struct & use it to pass all cluster related params #772

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[2/5] cluster: refactor param passing & add params to the builder #670

[2/5] cluster: refactor param passing & add params to the builder #670

utkarshgupta137 commented Aug 23, 2022 •

edited

utkarshgupta137 commented Aug 31, 2022

jaymell commented Sep 7, 2022

djc left a comment

djc Sep 8, 2022

utkarshgupta137 Sep 8, 2022

djc Sep 8, 2022

utkarshgupta137 Sep 8, 2022

djc Sep 8, 2022

utkarshgupta137 Sep 8, 2022

djc Sep 8, 2022

utkarshgupta137 Sep 8, 2022

djc Sep 8, 2022

utkarshgupta137 Sep 8, 2022

djc Sep 8, 2022

utkarshgupta137 Sep 8, 2022

djc Sep 8, 2022

utkarshgupta137 Sep 8, 2022

djc Sep 8, 2022

utkarshgupta137 Sep 8, 2022

jaymell commented Sep 21, 2022

utkarshgupta137 commented Sep 21, 2022

jaymell commented Sep 21, 2022

djc Sep 22, 2022

utkarshgupta137 Sep 23, 2022

djc Sep 22, 2022

djc Sep 22, 2022

djc commented Sep 26, 2022

		@@ -64,6 +64,12 @@ use crate::cluster_routing::{Routable, RoutingInfo, Slot, SLOT_SIZE};

		type SlotMap = BTreeMap<u16, [String; 2]>;

		#[derive(Clone, Copy)]

[2/5] cluster: refactor param passing & add params to the builder #670

[2/5] cluster: refactor param passing & add params to the builder #670

Conversation

utkarshgupta137 commented Aug 23, 2022 • edited

utkarshgupta137 commented Aug 31, 2022

jaymell commented Sep 7, 2022

djc 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jaymell commented Sep 21, 2022

utkarshgupta137 commented Sep 21, 2022

jaymell commented Sep 21, 2022

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

djc commented Sep 26, 2022

utkarshgupta137 commented Aug 23, 2022 •

edited