Go: Implement Transaction #3518

EdricCua · 2025-04-03T13:02:15Z

Issue link

This Pull Request is linked to issue (URL): #3484

Checklist

Before submitting the PR make sure the following are checked:

This Pull Request is related to one issue.
Commit message has a detailed description of what changed and why.
Tests are added or updated.
CHANGELOG.md and documentation files are updated.
Destination branch is correct - main or release
Create merge commit if merging release branch into main, squash otherwise.

Signed-off-by: EdricCua <ecuartero@google.com>

shohamazon

Glide 2.0 will introduce support for batch execution (pipelining) alongside transactions.

Transactions allow executing a batch of commands atomically in a single request—ensuring that no other command interrupts the execution. See Valkey Transactions
Pipelining also executes a batch of commands in a single request but is not atomic. In cluster mode, pipelining allows commands with keys from different slots, unlike transactions. See Valkey Pipelines

Impact on Glide

This change requires significant updates in both the core implementation and language wrappers.

Core Changes

Support for handling pipeline requests is in progress and is expected to be merged soon.
Due to the complexity, the work was split across multiple PRs and merged into a side branch. ( Will be merged into main very soon!)

Wrapper Changes

The existing Transaction API will be deprecated.
A new Batch API will be introduced, featuring an isAtomic flag to indicate whether the batch is a transaction or a pipeline.

This means renaming:

BaseTransaction → BaseBatch
Transaction → Batch
ClusterTransaction → ClusterBatch
These changes require significant updates across all wrappers.

Go Wrapper Considerations

Since the Go wrapper is not fully released and does not yet support transactions, adding transactions now only to replace them with batches before release doesn't make sense.

Instead, we should implement batch support first to minimize future breaking changes and avoid introducing deprecations in Go before release.

Next Steps

This is a major change across all wrappers, and we want to ensure Glide remains the best client available. I know it will take time, but your work is greatly appreciated!

Let’s start with batch support (instead of transactions) to ensure a smoother transition.

Thank you very much! 🙂

Signed-off-by: EdricCua <ecuartero@google.com>

shohamazon · 2025-04-06T08:33:30Z

ffi/src/lib.rs

+}
+
+#[no_mangle]
+pub unsafe extern "C" fn execute_transaction(


please notice #3521 as it add changes in socket_listeners.rs :)

and also in protobuf files

please note that with that you will probably need to add support for Value::ServeError return type :)

As discussed with @shohamazon Retry_server_error and Retry_connection_error are not ready at the moment mod.rs. This should be part of subsequent releases.

Retry_server_error - not ready for transaction
Retry_connection_error - not ready for transaction

@shohamazon I have added the Value::ServerError , now it returns the array with error and it continues with command execution for other remaining commands.

please add raise on error + a test where i can see the error being returned

if you added one, please tag me there, as I am only reviewing the ffi and not the go part

Can you explain the discussion, pls?
We are going to introduce new error handling in the following release?

shohamazon · 2025-04-06T08:39:18Z

ffi/src/lib.rs

+    route_bytes_len: usize,
+) -> *mut CommandResult {
+    if transaction.is_null() {
+        panic!("Transaction pointer is NULL");


is it safe to add all of this panics?

use failure callback to report failure

we used .expect() instead.

except behaves like panics

@shohamazon , We can see expect is being used widely in lib.rs so we are following the same approach. In future, if there is support for failure callback then this can be also updated at the same time.

ffi/src/lib.rs

Signed-off-by: EdricCua <ecuartero@google.com>

Signed-off-by: Niharika Bhavaraju <nbhavaraju@google.com>

niharikabhavaraju · 2025-05-05T17:10:48Z

I am lacking some important things:

raise_on_error tests should run against cluster mode enabled and cluster mode disabled (GlideClient and GlideClusterClient).
More than that, it cant just run agains raise_on_error set to true, we need to test false as well and see what the output.
And lastly, we must check the error type (in true and false as well), it cant just be a generic error, it should be a RequestError

This test can really help you with it:

valkey-glide/java/integTest/src/test/java/glide/SharedCommandTests.java

Line 15536 in 2d04409

public void batch_raise_on_error(BaseClient client, boolean isAtomic) {

You can see that its running on both cluster mode and standalone, and that its running with true and false. This test also contains multiple errors, so we can make sure we get all of them. Just make sure you're copying the exact test, it should be enough.

timeout tests, I dont see any, this can help you:

valkey-glide/java/integTest/src/test/java/glide/SharedCommandTests.java

Line 15482 in 2d04409

public void batch_timeout(BaseClient client, boolean isAtomic) {

Again, copying this should be enough 🙂

Note: I am not a go expert, so tell me if I missed something

This prs can help you: #3561 #3702 #3555

Hi @shohamazon ,
Now, we are sending a request error , please check here . All the comments have been addressed, can you please provide the approval.

shohamazon

I dont really understand why we have so many duplications of tests for cluster mode and standalone, is it a go thing? cant we use the same test for both standalone and cluster?

shohamazon · 2025-05-06T07:29:29Z

ffi/src/lib.rs

        }
+
+        Value::ServerError(server_error) => {
+            let code = server_error.err_code();


use the cores error_message func like in other lib.rs's

Have updated it into error_message link and adjusted the integration test and example file.

shohamazon · 2025-05-06T07:30:34Z

ffi/src/lib.rs

+/// `raise_on_error` When set to false, errors will be included as part of the batch response, allowing the caller to process both successful and failed commands together. In this case, error details
+///                  When set to true, the first encountered error in the batch will be raised as an exception.


in this case what? the line is cut in the middle

Already fixed, added the missing lines.

shohamazon · 2025-05-06T07:30:53Z

ffi/src/lib.rs

+    let transaction = unsafe { transaction.as_ref() }.expect("Transaction pointer is NULL");
+
+    let _commands =
+        unsafe { transaction.commands.as_ref() }.expect("Transaction commands pointer is NULL");


@avifenesh @jonathanl-bq , what do you think?

shohamazon · 2025-05-06T07:31:40Z

ffi/src/lib.rs

+        let mut cmd = cmder
+            .request_type
+            .get_command()
+            .expect("Invalid command type");


see for example an expect without unsafe, so your claim is still not enough

Can you please help navigate to any example where expect() is already taken care by other ways, it would be good as discussed yesterday.
Or else currently we can keep expect() and once it gets updated across all[ lib.rs](https://github.com/EdricCua/valkey-glide/blob/Go-Implement-Transaction-BaseLine/ffi/src/lib.rs, it can be refactored everywhere. @shohamazon

shohamazon · 2025-05-06T07:34:47Z

go/integTest/transaction_test.go

+	}
+
+	_, err := tx.ExecWithOptions(options)
+	assert.Error(suite.T(), err)


can you check the error type please?

yep we are checking the error type here

what I mean is, checking the instance of the error, to be instance of

valkey-glide/go/api/errors/errors.go

Line 31 in 80d17cd

type TimeoutError struct {

shohamazon · 2025-05-06T07:38:35Z

go/integTest/transaction_test.go

+	assert.Contains(suite.T(), resultString, "OK")
+}
+
+func (suite *GlideTestSuite) TestExecWithOptions_Cluster_RaiseOnErrorFalse() {


same comment I had for raise on error true

already adjusted, same with the raise on error true

shohamazon · 2025-05-06T07:39:24Z

go/integTest/transaction_test.go

+	options = &api.TransactionOption{
+		Timeout: 1000,
+	}
+


add a sleep before the next execution

Added Go Sleep before execution

shohamazon · 2025-05-06T07:40:02Z

go/integTest/transaction_test.go

+	options = &api.TransactionOption{
+		Timeout: 1000,
+	}
+


add a sleep before the next execution

Added Go Sleep before execution

shohamazon · 2025-05-06T07:40:26Z

go/integTest/transaction_test.go

+	assert.Equal(suite.T(), &errors.RequestError{Msg: "ERR - no such key"}, results[3])
+}
+
+func (suite *GlideTestSuite) TestExecWithOptions_RaiseOnErrorTrue() {


same comments for raise on error as bellow

already adjusted, same with the raise on error

shohamazon · 2025-05-06T07:40:31Z

go/integTest/transaction_test.go

+	assert.Contains(suite.T(), err.Error(), "WRONGTYPE")
+}
+
+func (suite *GlideTestSuite) TestExecWithOptions_RaiseOnErrorFalse() {


same comments for raise on error as bellow

already adjusted, same with the raise on error

Signed-off-by: EdricCua <ecuartero@google.com>

shohamazon

LGTM, small comments, waiting for the teams review 🙂

shohamazon · 2025-05-07T08:20:51Z

ffi/src/lib.rs

+
+/// CGO method which allows the Go client to executes all queued commands as a transaction
+///
+/// `client_adapter_ptr` is a pointer to a valid `GlideClusterClient` returned in the `ConnectionResponse` from [`create_client`].


minor: GlideClient or GlideClusterClient

shohamazon · 2025-05-07T08:29:38Z

go/integTest/transaction_test.go

+	}
+
+	_, err := tx.ExecWithOptions(options)
+	assert.Error(suite.T(), err)


what I mean is, checking the instance of the error, to be instance of

valkey-glide/go/api/errors/errors.go

Line 31 in 80d17cd

type TimeoutError struct {

shohamazon · 2025-05-07T08:31:26Z

go/integTest/transaction_test.go

+		simpleRoute,
+	)
+
+	options := &api.TransactionOption{


yes I am taking about the exec function receiving the options, so cluster mode options and standalone mode options should be different
see here https://github.com/valkey-io/valkey-glide/tree/main/java/client/src/main/java/glide/api/models/commands/batch

shohamazon · 2025-05-07T08:31:52Z

go/integTest/transaction_test.go

+	assert.Equal(suite.T(), int64(1), results[2])
+	assert.Equal(
+		suite.T(),
+		&errors.RequestError{Msg: "An error was signalled by the server: - ResponseError: no such key"},


shohamazon · 2025-05-07T08:32:04Z

go/integTest/transaction_test.go

+	_, err := tx.ExecWithOptions(options)
+
+	assert.Error(suite.T(), err)
+	assert.Contains(suite.T(), err.Error(), "WRONGTYPE")


check error type

shohamazon · 2025-05-07T08:36:55Z

go/api/transaction.go

+}
+
+// TransactionOptions contains configurable options for transaction execution
+type TransactionOption struct {


so like here : https://github.com/valkey-io/valkey-glide/tree/main/java/client/src/main/java/glide/api/models/commands/batch

I want us to have different options types for cluster mode and standalone mode

like this:

BaseOptions for options like raise on error and timeout (both standalone and cluster)
ClusterOptions for options that are just suitable for cluster, like route and retry strategy (in the future ofc)
and StandaloneOptions for options that are just for standalone, I know currently we dont have some, but we might have in the future, so we better have different types to avoid breaking changes in the future

the ClusterOptions and StandaloneOptions should inherit from BaseOptions

jbrinkman

We should hold this PR until we have final agreement on the use of context.Context in the Go client implementation. Hope to get agreement between Improving/Amazon/Google teams in next day or two.

jbrinkman · 2025-05-07T12:12:50Z

go/api/transaction.go

+
+type Transaction struct {
+	*baseClient // Embed baseClient to inherit all methods like Get
+	*GlideClient


This struct doesn't make sense. Both GlideClient and GlideClusterClient already embed *baseClient. As @marcind points out below, transactions are tied to the client type so a given transaction should only have access to either a GlideClient or a GlideClusterClient, but not both. The baseClient is redundant.

jbrinkman · 2025-05-07T12:17:28Z

go/api/example_utils.go

+func getExampleTransactionGlideClient() *Transaction {
+	standaloneTransactionOnce.Do(func() {
+		initFlags()
+		addresses := parseHosts(*standaloneNode)


Why are you reparsing the standaloneNodes again? This is already done in the initFlags. It looks like maybe this was based on the original implementation of the sync.Once pattern which has since been refactored.

jbrinkman · 2025-05-07T12:30:28Z

go/api/example_utils.go

+		if errFlush != nil {
+			fmt.Println("error flushing database: ", err)
+		}
+		clientTx := NewTransaction(client)


This looks like it is using the original pattern of wrapping the entire client creation logic in the sync.Once singleton pattern. What I found with PubSub is that this was overly aggressive since it meant that every example was using the exact same client configuration, and the client was not getting flushed between examples. You should remove the sync.Once implementation here, and just rely on the sync.Once in initFlags() to minimize reparsing flags (flag.Parse() can't be called multiple times). I'll be creating a separate PR to remove the other sync.Once object usage outside of initFlags

jbrinkman · 2025-05-07T12:39:26Z

go/api/transaction.go

+}
+
+// Exec executes all queued commands as a transaction
+func (t *Transaction) Exec() ([]any, error) {


@marcind We are having a separate discussion on this so we can address how we want to handle this across the entire client. Thank you for bringing this up in issue #3709

jbrinkman · 2025-05-07T13:52:25Z

go/api/transaction_test.go

+	cmd := clientTx.GlideClient
+	clientTx.Discard()
+	cmd.Set("key123", "Glide")
+	cmd.Watch([]string{"key123", "key345"})


While this example shows the watch command being used, it is not at all clear how the behavior in this example would change if the cmd.Watch was not included. I would like to see a case where we are specifically showing a case where Watch was used and the value of key123 mutated during the transaction, or where we were specifically dealing with this error condition. For example, in this Go-Redis code from the Redis docs, you can see that the example handles the case where a key is mutated.

// Set initial value of `shellpath`. rdb.Set(ctx, "shellpath", "/usr/syscmds/", 0) const maxRetries = 1000 // Retry if the key has been changed. for i := 0; i < maxRetries; i++ { err := rdb.Watch(ctx, func(tx *redis.Tx) error { currentPath, err := rdb.Get(ctx, "shellpath").Result() newPath := currentPath + ":/usr/mycmds/" _, err = tx.TxPipelined(ctx, func(pipe redis.Pipeliner) error { pipe.Set(ctx, "shellpath", newPath, 0) return nil }) return err }, "shellpath", ) if err == nil { // Success. break } else if err == redis.TxFailedErr { // Optimistic lock lost. Retry the transaction. continue } else { // Panic for any other error. panic(err) } } fmt.Println(rdb.Get(ctx, "shellpath").Val()) // >>> /usr/syscmds/:/usr/mycmds/ ```

Bonus points for an example that shows how to handle broken TCP pipes, CLUSTERDOWN, MASTERDOWN, and other transient errors.

avifenesh

Removing my block

shohamazon

Removing my block 🍒

Yury-Fridlyand · 2025-05-21T22:40:21Z

Superseded by #3938

EdricCua added 15 commits March 24, 2025 16:38

Implement transaction baseline

8b98903

Signed-off-by: EdricCua <ecuartero@google.com>

Implement transaction baseline

1e5c6bd

Signed-off-by: EdricCua <ecuartero@google.com>

Implement transaction basseline

8806735

Signed-off-by: EdricCua <ecuartero@google.com>

Implement transaction

7175f7f

Signed-off-by: EdricCua <ecuartero@google.com>

implement transaction

c47553b

Signed-off-by: EdricCua <ecuartero@google.com>

Implement transaction

fe9e80f

Signed-off-by: EdricCua <ecuartero@google.com>

Implement transaction

7a1b598

Signed-off-by: EdricCua <ecuartero@google.com>

Implement transaction

b5ed105

Signed-off-by: EdricCua <ecuartero@google.com>

update from main

5e8f8b6

Signed-off-by: EdricCua <ecuartero@google.com>

update from main

01a4b86

Signed-off-by: EdricCua <ecuartero@google.com>

update from main

3728f76

Signed-off-by: EdricCua <ecuartero@google.com>

update from main

185fc44

Signed-off-by: EdricCua <ecuartero@google.com>

implement transaction

4aac800

Signed-off-by: EdricCua <ecuartero@google.com>

update from main

3a659e2

Signed-off-by: EdricCua <ecuartero@google.com>

implement transaction

3bb43d6

Signed-off-by: EdricCua <ecuartero@google.com>

EdricCua requested a review from a team as a code owner April 3, 2025 13:02

shohamazon requested changes Apr 3, 2025

View reviewed changes

Yury-Fridlyand added the go 🏃 golang wrapper label Apr 3, 2025

EdricCua added 5 commits April 4, 2025 10:21

Update transactions

c46d710

Signed-off-by: EdricCua <ecuartero@google.com>

Update transactions

b8d23e5

Signed-off-by: EdricCua <ecuartero@google.com>

Update transactions

cfd5d57

Signed-off-by: EdricCua <ecuartero@google.com>

Update transactions

3e8df72

Signed-off-by: EdricCua <ecuartero@google.com>

Update transactions

b47b043

Signed-off-by: EdricCua <ecuartero@google.com>

shohamazon reviewed Apr 6, 2025

View reviewed changes

ffi/src/lib.rs Show resolved Hide resolved

shohamazon reviewed Apr 6, 2025

View reviewed changes

ffi/src/lib.rs Outdated Show resolved Hide resolved

shohamazon reviewed Apr 6, 2025

View reviewed changes

ffi/src/lib.rs Outdated Show resolved Hide resolved

EdricCua added 2 commits April 7, 2025 02:40

Update transactions

bc400b4

Signed-off-by: EdricCua <ecuartero@google.com>

Update transactions

fa22044

Signed-off-by: EdricCua <ecuartero@google.com>

niharikabhavaraju added 5 commits May 5, 2025 12:56

Added/Modified tests

6f81f5a

Signed-off-by: Niharika Bhavaraju <nbhavaraju@google.com>

Merge branch 'main' into Go-Implement-Transaction-BaseLine

57830a4

Fix lint error

b9162d2

Signed-off-by: Niharika Bhavaraju <nbhavaraju@google.com>

Modified error message format for transaction

07401b3

Signed-off-by: Niharika Bhavaraju <nbhavaraju@google.com>

Merge branch 'main' into Go-Implement-Transaction-BaseLine

8a38a63

shohamazon reviewed May 6, 2025

View reviewed changes

EdricCua added 13 commits May 6, 2025 10:47

Fix review comment

760364d

Signed-off-by: EdricCua <ecuartero@google.com>

Fix in lib.rs

b90c136

Signed-off-by: EdricCua <ecuartero@google.com>

Fix in lib.rs

e3905ac

Signed-off-by: EdricCua <ecuartero@google.com>

Fix in lib.rs

d546257

Signed-off-by: EdricCua <ecuartero@google.com>

Fix in lib.rs

071ff25

Signed-off-by: EdricCua <ecuartero@google.com>

Fix in transaction_test

2ce039d

Signed-off-by: EdricCua <ecuartero@google.com>

Docs for glide_client and glide_cluster

7916df7

Signed-off-by: EdricCua <ecuartero@google.com>

Fix examples

3295c94

Signed-off-by: EdricCua <ecuartero@google.com>

Fix examples

6cdf3b7

Signed-off-by: EdricCua <ecuartero@google.com>

Fix examples

d52a355

Signed-off-by: EdricCua <ecuartero@google.com>

Fix examples

4ccc146

Signed-off-by: EdricCua <ecuartero@google.com>

Fix in transaction_test

e06d3fd

Signed-off-by: EdricCua <ecuartero@google.com>

Fix in transaction_test

df1c85c

Signed-off-by: EdricCua <ecuartero@google.com>

shohamazon reviewed May 7, 2025

View reviewed changes

jbrinkman requested changes May 7, 2025

View reviewed changes

avifenesh self-requested a review May 20, 2025 15:29

avifenesh reviewed May 20, 2025

View reviewed changes

shohamazon self-requested a review May 20, 2025 15:30

shohamazon reviewed May 20, 2025

View reviewed changes

avifenesh approved these changes May 20, 2025

View reviewed changes

shohamazon approved these changes May 20, 2025

View reviewed changes

Yury-Fridlyand closed this May 21, 2025

		/// `raise_on_error` When set to false, errors will be included as part of the batch response, allowing the caller to process both successful and failed commands together. In this case, error details
		/// When set to true, the first encountered error in the batch will be raised as an exception.

Go: Implement Transaction #3518

Go: Implement Transaction #3518

Uh oh!

Conversation

EdricCua commented Apr 3, 2025

Issue link

Checklist

Uh oh!

shohamazon left a comment

Choose a reason for hiding this comment

Impact on Glide

Core Changes

Wrapper Changes

Go Wrapper Considerations

Next Steps

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

niharikabhavaraju Apr 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

EdricCua Apr 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

niharikabhavaraju commented May 5, 2025

Uh oh!

shohamazon left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

niharikabhavaraju Apr 24, 2025 •

edited

Loading

EdricCua Apr 23, 2025 •

edited

Loading