Go: Implement connect #136

aaron-congo · 2024-03-11T16:43:58Z

adds lib.rs and Go logic to establish standalone and cluster connections
adds integration test logic to setup/teardown redis servers
adds integration tests for connect workflows

go/api/base_client.go

SanHalacogluImproving

Seems like this PR is too big to go to upstream why not push lib.rs first get the review on it and then push the go code?

go/api/base_client.go

SanHalacogluImproving · 2024-03-14T17:58:59Z

go/api/base_client.go

+		return errors.New("close called on uninitialized or already closed client")
+	}
+
+	C.close_client(client.coreClient)


This is something in the future to think about but what happens when a client is closed and a command gets called? Assuming the pointer will be null we probably need to handle it on the rust or go side.

Yeah the other clients check if the client is already closed before attempting to execute a command and throw a ClosingError if they are, so we should do something similar if coreClient is nil. Note that Shachar said ClosingError is only applicable to UDS clients (see here) so we'd return something different (probably DisconnectError)

valkey-io#1077 (comment) I think it should be a closing error.

What if client get closed while a command being executed? We should protect vs application crash in that case too.
My idea so far is to add one more argument to callbacks - client id. A callback submitted to rust would be a static function then, which can dispatch the response to a client if it is not closed yet or ignore vice versa.
TBD

go/integTest/error_handling_test.go

aaron-congo · 2024-03-14T21:19:33Z

Seems like this PR is too big to go to upstream why not push lib.rs first get the review on it and then push the go code?

I think I brought up this idea before and IIRC Andrew thought they wouldn't want the lib.rs without any integration tests. The PR isn't small but it isn't massive either, I think I will just open it against upstream once this is approved and see if they ask me to split the lib.rs out

.github/workflows/go.yml

Yury-Fridlyand · 2024-03-18T16:28:43Z

go/api/base_client.go

+		return errors.New("close called on uninitialized or already closed client")
+	}
+
+	C.close_client(client.coreClient)


What if client get closed while a command being executed? We should protect vs application crash in that case too.
My idea so far is to add one more argument to callbacks - client id. A callback submitted to rust would be a static function then, which can dispatch the response to a client if it is not closed yet or ignore vice versa.
TBD

go/integTest/setup_teardown_test.go

Yury-Fridlyand · 2024-03-18T16:35:01Z

go/integTest/setup_teardown_test.go

+	clusterManagerOutput := runClusterManager(suite, []string{"start", "-r", "0"}, false)
+
+	suite.standalonePorts = extractPorts(suite, clusterManagerOutput)
+	suite.T().Logf("Standalone ports = %s", fmt.Sprint(suite.standalonePorts))


Sorry for stupid golang question - what does suite.T() do? Where it is defined?

For tests that aren't part of a test suite, you need to pass a parameter of type testing.T which provides a test context and methods for printing and logging (more info here). Here we are using a test suite, and you access testing.T via suite.T() instead of as a direct parameter. This is mentioned in Testify's docs here

.github/workflows/go.yml

go/integTest/connection_test.go

Yury-Fridlyand · 2024-03-18T16:39:33Z

go/src/lib.rs

@@ -1,9 +1,159 @@
+#![deny(unsafe_op_in_unsafe_fn)]


Move this after license header

The Rust code exists in this PR to get tests passing but we are reviewing it under a separate PR here: https://github.com/aws/glide-for-redis/pull/1127/files

aaron-congo · 2024-03-20T21:11:33Z

go/api/config.go

@@ -140,8 +140,7 @@ func (strategy *BackoffStrategy) toProtobuf() *protobuf.ConnectionRetryStrategy
 	}
 }

-// RedisClientConfiguration represents the configuration settings for a Standalone Redis client. baseClientConfiguration is an
-// embedded struct that contains shared settings for standalone and cluster clients.
+// RedisClientConfiguration represents the configuration settings for a Standalone Redis client.


I realized that the final documentation does not show the unexported fields, so it doesn't make sense to mention baseClientConfiguration here

aaron-congo · 2024-03-20T21:15:17Z

go/api/standalone_client.go

+}
+
+// NewRedisClient creates a Redis client in standalone mode using the given [RedisClientConfiguration].
+func NewRedisClient(config *RedisClientConfiguration) (*RedisClient, error) {


In Java this is called CreateClient, but in Go the idiom is that a function called NewRedisClient should be provided if users aren't supposed to initialize a RedisClient struct directly. Also, the Go docs normally will document exported symbols alphabetically but will place New functions immediately underneath the document for the associated struct. We lose this clarity if we call this CreateClient

aaron-congo · 2024-07-17T17:51:21Z

Closing as this is superseded by another PR upstream (valkey-io#1937)

aaron-congo commented Mar 11, 2024

View reviewed changes

go/api/base_client.go Show resolved Hide resolved

go/api/base_client.go Outdated Show resolved Hide resolved

aaron-congo closed this Mar 11, 2024

aaron-congo reopened this Mar 13, 2024

SanHalacogluImproving reviewed Mar 14, 2024

View reviewed changes

aaron-congo force-pushed the go/dev_acongo_connect branch from 3764973 to 0d52ffa Compare March 14, 2024 20:01

jonathanl-bq approved these changes Mar 14, 2024

View reviewed changes

SanHalacogluImproving approved these changes Mar 15, 2024

View reviewed changes

Yury-Fridlyand reviewed Mar 18, 2024

View reviewed changes

aaron-congo force-pushed the go/dev_acongo_connect branch from 5b5add8 to 6fe92fb Compare March 20, 2024 15:44

aaron-congo commented Mar 20, 2024

View reviewed changes

aaron-congo force-pushed the go/integ_acongo_connect branch from 033fe5d to c4daccd Compare March 21, 2024 00:05

jonathanl-bq and others added 7 commits March 20, 2024 17:06

Address PR comments

b1e58d6

Go: implement connect

05f1ae0

Adjust Go code to new lib.h

7aad421

Rename CreateClient, fix doc

8126d7b

Fix indentation

980a36d

Update doc

2d9b990

Fix indentation

d7f10e3

aaron-congo force-pushed the go/dev_acongo_connect branch from 4720d29 to d7f10e3 Compare March 21, 2024 00:08

aaron-congo added 4 commits March 21, 2024 09:33

Fix Rust build

ee5e47a

Fix lint-rust

c0f2034

Rename interface

15094a4

Rename interface

49a3646

Yury-Fridlyand added the go label Apr 17, 2024

aaron-congo closed this Jul 17, 2024

aaron-congo deleted the go/dev_acongo_connect branch July 17, 2024 17:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Go: Implement connect #136

Go: Implement connect #136

aaron-congo commented Mar 11, 2024

SanHalacogluImproving left a comment

SanHalacogluImproving Mar 14, 2024

aaron-congo Mar 14, 2024 •

edited

Loading

SanHalacogluImproving Mar 15, 2024

Yury-Fridlyand Mar 18, 2024 •

edited

Loading

aaron-congo commented Mar 14, 2024 •

edited

Loading

Yury-Fridlyand Mar 18, 2024 •

edited

Loading

Yury-Fridlyand Mar 18, 2024

aaron-congo Mar 18, 2024 •

edited

Loading

Yury-Fridlyand Mar 18, 2024

aaron-congo Mar 18, 2024

aaron-congo Mar 20, 2024

aaron-congo Mar 20, 2024

aaron-congo commented Jul 17, 2024

Go: Implement connect #136

Go: Implement connect #136

Conversation

aaron-congo commented Mar 11, 2024

SanHalacogluImproving left a comment

Choose a reason for hiding this comment

SanHalacogluImproving Mar 14, 2024

Choose a reason for hiding this comment

aaron-congo Mar 14, 2024 • edited Loading

Choose a reason for hiding this comment

SanHalacogluImproving Mar 15, 2024

Choose a reason for hiding this comment

Yury-Fridlyand Mar 18, 2024 • edited Loading

Choose a reason for hiding this comment

aaron-congo commented Mar 14, 2024 • edited Loading

Yury-Fridlyand Mar 18, 2024 • edited Loading

Choose a reason for hiding this comment

Yury-Fridlyand Mar 18, 2024

Choose a reason for hiding this comment

aaron-congo Mar 18, 2024 • edited Loading

Choose a reason for hiding this comment

Yury-Fridlyand Mar 18, 2024

Choose a reason for hiding this comment

aaron-congo Mar 18, 2024

Choose a reason for hiding this comment

aaron-congo Mar 20, 2024

Choose a reason for hiding this comment

aaron-congo Mar 20, 2024

Choose a reason for hiding this comment

aaron-congo commented Jul 17, 2024

aaron-congo Mar 14, 2024 •

edited

Loading

Yury-Fridlyand Mar 18, 2024 •

edited

Loading

aaron-congo commented Mar 14, 2024 •

edited

Loading

Yury-Fridlyand Mar 18, 2024 •

edited

Loading

aaron-congo Mar 18, 2024 •

edited

Loading