refactor: changes to storage-related transaction handling within AutoOps packages #1365
Conversation
kakcy
force-pushed
the
update-storage-client-auto-ops
branch
from
db6b44e to
6e1ae57
Compare
…rogressiveRollout storage
pkg/storage/v2/mysql/client.go
Outdated
| @@ -199,3 +207,29 @@ func (c *client) RunInTransaction(ctx context.Context, tx Transaction, f func() | |||
| } | |||
| return err | |||
| } | |||
|
|
|||
| func (c *client) RunInTransactionV2(ctx *context.Context, f func(tx Transaction) error) error { | |||
There was a problem hiding this comment.
We don't need to use pointer here, since context.Context is interface already, reference: https://github.com/bucketeer-io/bucketeer/blob/main/pkg/account/storage/v2/storage.go#L66-L73
There was a problem hiding this comment.
@Ubisoft-potato
Thank you for review 👍
In this PR, Transaction instances are managed using Context.WithValue().
https://github.com/bucketeer-io/bucketeer/blob/update-storage-client-auto-ops/pkg/storage/v2/mysql/client.go#L212-L216
Also, each MySQLClient side uses Context.Value with Client.Qe() in between to check whether there is a Transaction instance and switch the instance to execute the query.
ex: AutoOpsStorage.CreateAutoOpsRule()
https://github.com/bucketeer-io/bucketeer/blob/update-storage-client-auto-ops/pkg/autoops/api/api.go#L195
I think that if no pointer is passed, the context will not be updated and the transaction instance will not be saved.
As a result, I think transactions won't work and each query will be executed separately.
What do you think?
There was a problem hiding this comment.
Hi @kakcy.
The current implementation of RunInTransactionV2 takes a pointer to context.Context which is problematic for several reasons:
-
Go Context Convention: Context should always be passed as a value, not as a pointer. This is a well-established Go convention and is documented in the context package.
-
Immutability Pattern: Context is designed to follow an immutability pattern. Instead of modifying an existing context, you should create a new one and pass it down the call chain.
-
Thread Safety: Modifying a context through a pointer could lead to race conditions in concurrent operations.
There was a problem hiding this comment.
Instead of passing tx here, we can pass ctx:
func (c *client) RunInTransactionV2(ctx context.Context, f func(ctx context.Context) error) error {
ctx = context.WithValue(ctx, transactionKey, tx)
if err = f(ctx); err == nil {
err = tx.Commit()
}
}So we can still access the newset tx in the ctx.
There was a problem hiding this comment.
Hi @kakcy , I want to quote a small paragraph in https://www.oreilly.com/library/view/concurrency-in-go/9781491941294/ (it's a very good book)
This book has a chapter about context package, and it notes that we should not pass by reference but using the instance instead. As the book also states, go philosophy is "sharing by communicating, not communicating by sharing", so maybe we can find a way to not update the context ctx but using its tree-forked paradigm to share the transaction tx as @Ubisoft-potato stated?
What do you think?
There was a problem hiding this comment.
Thank you for your detailed explanations.
I will modify it to match the philosophy of the Golang.
pkg/storage/v2/mysql/client.go
Outdated
| if err != nil { | ||
| return fmt.Errorf("account: begin tx: %w", err) | ||
| } | ||
| *ctx = context.WithValue(*ctx, transactionKey, tx) |
There was a problem hiding this comment.
Same here:
ctx = context.WithValue(ctx, transactionKey, tx)
|
@Ubisoft-potato @hvn2k1 |
|
LGTM now! 👍 |
|
LGTM too! Nice work! |
This pull request includes significant changes to the
AutoOpsServiceandProgressiveRolloutfunctionalities, focusing on refactoring the transaction handling mechanism. The primary change is the transition from usingRunInTransactiontoRunInTransactionV2, which simplifies the transaction management and improves code maintainability.Part of #1252