Add support for client name configuration #71

ikolomi · 2023-12-10T11:59:03Z

Issue #, if available:
Part of valkey-io/valkey-glide#407

Description of changes:
Add optional client_name property to RedisConnectionInfo, which will be used with 'CLIENT SETNAME' command during connection setup.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

shachlanAmazon

round

redis/src/aio/mod.rs

redis/src/connection.rs

redis/tests/support/mod.rs

redis/tests/test_basic.rs

barshaul · 2023-12-11T15:11:19Z

redis/tests/test_async.rs

+                    let this_attr: Vec<&str> = i.split('=').collect();
+                    if this_attr[0] == "name" {
+                        assert!(
+                            this_attr[1] == CLIENT_NAME,


We are planning to have additional connection for each node ('management' connection). We can't be sure it's the first client connection we find.

You can take an example from this test:
https://github.com/amazon-contributing/redis-rs/pull/56/files#diff-85259ca2185589cf8979102fc91c6ce1a9461aac8ed605472b9ad53c3324c400R1912

it is not the first client, its the first that has the name set. During the test it is reasonable to require that no other client has set its name.

Example for what please? What is the issue we are talking about?

Summarizing our talk yesterday - the test will be changed to check the client ID and based on the it would check that its name is the one passed in the config

barshaul · 2023-12-14T14:49:16Z

redis/tests/test_cluster.rs

+        false,
+    );
+    let mut con = cluster.connection();
+    let client_info: String = redis::cmd("CLIENT").arg("INFO").query(&mut con).unwrap();


i'm not sure if client info is being sent to all nodes, have you checked it?

I think routing it to all nodes would cover the functionality of this feature in cluster better

you can't route manually in sync cluster.

We think that randomly checking a single node is good enough (since we cant route to all)

but you can route to all in async cluster

redis/src/cluster_client.rs

shachlanAmazon · 2023-12-14T15:27:57Z

redis/src/cluster_client.rs

@@ -212,6 +222,13 @@ impl ClusterClientBuilder {
                )));
            }

+            if client_name.is_some() && node.redis.client_name != *client_name {


IMO we should return an error if node.redis.client_name is set, regardless of client_name it should be an error, because it might confuse users - they'll think that setting the client name on the initial nodes will affect the whole cluster, but it won't survive any change to the cluster. WDYT?

I see that the same mistake is made with the username and password, so this can be fixed in a separate PR.

redis/tests/support/cluster.rs

shachlanAmazon · 2023-12-14T15:31:05Z

redis/tests/test_async.rs

+        for attr in client_attrs.iter() {
+            let this_attr: Vec<&str> = attr.split('=').collect();
+            if this_attr[0] == "name" {
+                assert!(


you can use assert_eq!, it logs the different values so there's no need for the extra message.

shachlanAmazon · 2023-12-14T15:31:14Z

redis/tests/test_basic.rs

+    for attr in client_attrs.iter() {
+        let this_attr: Vec<&str> = attr.split('=').collect();
+        if this_attr[0] == "name" {
+            assert!(


shachlanAmazon · 2023-12-14T15:34:56Z

redis/tests/test_async.rs

+            .await
+            .unwrap();
+
+        let client_attrs: Vec<&str> = client_info.split(' ').collect();


you're using the same logic, at the very least move the splitting into attribute pairs to a separate function. If you really want to make it handy, have it return HashMap<String, String>.

Also, there's no need for the collecting. for statements work on iterators:

for (key, value) in client_info.split(' ').map(|line| attr.split('=').collect::<(&str, &str)>()) {

should be sufficient.

shachlanAmazon · 2023-12-14T15:37:13Z

redis/tests/test_cluster.rs

+        false,
+    );
+    let mut con = cluster.connection();
+    let client_info: String = redis::cmd("CLIENT").arg("INFO").query(&mut con).unwrap();


you can't route manually in sync cluster.

redis/Cargo.toml

shachlanAmazon · 2023-12-18T12:06:03Z

redis/src/cluster_client.rs

@@ -191,6 +194,13 @@ impl ClusterClientBuilder {
            };
        }

+        let client_name = if cluster_params.client_name.is_none() {


remove, as discussed

shachlanAmazon · 2023-12-18T12:07:58Z

redis/src/cluster_client.rs

@@ -212,6 +222,13 @@ impl ClusterClientBuilder {
                )));
            }

+            if client_name.is_some() && node.redis.client_name != *client_name {


I think we said it should be if client_name != node.redis.client_name {. rigtht?

shachlanAmazon · 2023-12-18T12:08:24Z

redis/src/cluster_client.rs

@@ -221,6 +238,12 @@ impl ClusterClientBuilder {
        })
    }

+    /// Sets password for the new ClusterClient.


fix comment

shachlanAmazon

all comments are minor. approved.

shachlanAmazon · 2023-12-18T12:11:44Z

redis/tests/test_async.rs

+
+        let client_attrs = parse_client_info(&client_info);
+
+        assert!(


minor: this is essentially contained in the next assert, IMO can be removed

shachlanAmazon · 2023-12-18T12:11:49Z

redis/tests/test_basic.rs

+
+    let client_attrs = parse_client_info(&client_info);
+
+    assert!(


shachlanAmazon · 2023-12-18T12:12:00Z

redis/tests/test_cluster.rs

+
+    let client_attrs = parse_client_info(&client_info);
+
+    assert!(


shachlanAmazon · 2023-12-18T12:12:17Z

redis/tests/test_cluster_async.rs

+
+        let client_attrs = parse_client_info(&client_info);
+
+        assert!(


…be used with 'CLIENT SETNAME' command during connection setup.

barshaul

Overall approved,
But I do think the async cluster test should route to all nodes. If it's not a lot of work adding that - please do 🕺🕺

barshaul requested review from barshaul and shachlanAmazon December 10, 2023 13:03

shachlanAmazon reviewed Dec 10, 2023

View reviewed changes

barshaul reviewed Dec 11, 2023

View reviewed changes

barshaul reviewed Dec 14, 2023

View reviewed changes

shachlanAmazon reviewed Dec 14, 2023

View reviewed changes

shachlanAmazon reviewed Dec 18, 2023

View reviewed changes

shachlanAmazon approved these changes Dec 18, 2023

View reviewed changes

Add optional client_name property to RedisConnectionInfo, which will …

41e6e19

…be used with 'CLIENT SETNAME' command during connection setup.

barshaul approved these changes Dec 18, 2023

View reviewed changes

ikolomi merged commit 896fce1 into amazon-contributing:main Dec 18, 2023
10 checks passed

ikolomi deleted the add_clientname_support branch December 18, 2023 19:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for client name configuration #71

Add support for client name configuration #71

ikolomi commented Dec 10, 2023

shachlanAmazon left a comment

barshaul Dec 11, 2023

barshaul Dec 11, 2023

ikolomi Dec 11, 2023 •

edited

Loading

ikolomi Dec 11, 2023

barshaul Dec 12, 2023

barshaul Dec 14, 2023

barshaul Dec 14, 2023

shachlanAmazon Dec 14, 2023

ikolomi Dec 18, 2023 •

edited

Loading

barshaul Dec 18, 2023

shachlanAmazon Dec 14, 2023

shachlanAmazon Dec 14, 2023

shachlanAmazon Dec 14, 2023

shachlanAmazon Dec 14, 2023

shachlanAmazon Dec 14, 2023

shachlanAmazon Dec 18, 2023

shachlanAmazon Dec 18, 2023

shachlanAmazon Dec 18, 2023

shachlanAmazon left a comment

shachlanAmazon Dec 18, 2023

shachlanAmazon Dec 18, 2023

shachlanAmazon Dec 18, 2023

shachlanAmazon Dec 18, 2023

barshaul left a comment

Add support for client name configuration #71

Add support for client name configuration #71

Conversation

ikolomi commented Dec 10, 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

ikolomi Dec 11, 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

ikolomi Dec 18, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

barshaul left a comment

Choose a reason for hiding this comment

ikolomi Dec 11, 2023 •

edited

Loading

ikolomi Dec 18, 2023 •

edited

Loading