-
Notifications
You must be signed in to change notification settings - Fork 1.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(datastore): Adding BeginLater transaction option implementation #8972
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Initial review with a few comments and questions
4e5dda0
to
4943983
Compare
Do not merge until release freeze ends in mid April |
Does this client support retry transactions? Like before any of these changes, if the user provides an id when creating a transaction then will calling a read function make a grpc read request with that transaction id like it does in Node Datastore? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added one question about retry transactions to make sure there is parity with the Node client.
before and after any of these changes, no. Go client did not have retry transaction support |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM overall. Just a few nits and questions.
return func() {} | ||
} | ||
|
||
func (t *Transaction) startProgress(id []byte) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe change the function name to setToInProgress
or something like that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Resolved in #10032
settings: &transactionSettings{}, | ||
}, | ||
{ | ||
desc: "[RunAggregationQuery, Get, Put, Commit] BeginLater. RunAggregationQuery passes new_transaction", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like this test verifies that runAggregationQuery will pass in new transaction options. Just for my own knowledge though, what change in this PR allows newTransaction options to be passed in for the runAggregationQuery call? Does runAggregationQuery make a call to the new parseReadOptions()
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
RunAggregationQuery makes call to parseQueryReadOptions which calls parseReadOptions
@@ -88,3 +88,398 @@ func TestNewTransaction(t *testing.T) { | |||
} | |||
} | |||
} | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Other tests to consider if necessary:
- Make sure readTime is passed along for read only transactions
- Make sure prevId is passed along and used for read/write transactions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Tested here: https://github.com/googleapis/google-cloud-go/pull/8972/files#diff-8a638c1c614640f81c811cc62b5e9bacbb8b17493e158922ccb9ee004d4ffe70R70-R80
- prevId is not being used anywhere in Go client library.
@@ -889,6 +886,8 @@ func TestAggregationQueryIsNil(t *testing.T) { | |||
} | |||
|
|||
func TestValidateReadOptions(t *testing.T) { | |||
eventualInTxnErr := errors.New("datastore: cannot use EventualConsistency query in a transaction") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Since you're using this exact error in testing as you are in the code, would it be worth it to have this variable be defined in the code so you can import it in the test package, similar to errExpiredTransaction
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Resolved in #10063
return nil | ||
} | ||
|
||
// Acquire state lock if transaction has not started |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you add to the comment for acquireLock
more about what it is, what its return value signifies, and how it's used in other parts of the PR? It does a lot more than simply acquiring the state lock.
Also because this function does a lot more than just acquiring the state lock, could you rename the function to reflect that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Resolved in #10063
What is new_transaction ?
ReadOptions are the options shared by Datastore read requests.
new_transaction is a consistency type in ReadOptions. When new_transaction is used in a read request, a new transaction is begun for the request.
This PR adds following:
Currently, when user calls NewTransaction method, client library makes a BeginTransaction rpc call and obtains transaction id. Reads (RunQuery / RunAggregationQuery / LookupRequest), Commit and Rollback use this transaction id in ReadOptions of rpc request.
Without BeginLater transaction option: the above behavior remains unchanged.
With BeginLater transaction option:
Since there is one less round trip to the Datastore service in point 1 above, user may see lesser latencies when using BeginLater transaction option.