feat(datastore): Adding BeginLater transaction option implementation #8972
Conversation
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? |
before and after any of these changes, no. Go client did not have retry transaction support |
| return func() {} | ||
| } | ||
|
|
||
| func (t *Transaction) startProgress(id []byte) { |
There was a problem hiding this comment.
Maybe change the function name to setToInProgress or something like that?
| settings: &transactionSettings{}, | ||
| }, | ||
| { | ||
| desc: "[RunAggregationQuery, Get, Put, Commit] BeginLater. RunAggregationQuery passes new_transaction", |
There was a problem hiding this comment.
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.
RunAggregationQuery makes call to parseQueryReadOptions which calls parseReadOptions
| @@ -88,3 +88,398 @@ func TestNewTransaction(t *testing.T) { | |||
| } | |||
| } | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
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.
- 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.
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?
| return nil | ||
| } | ||
|
|
||
| // Acquire state lock if transaction has not started |
There was a problem hiding this comment.
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?
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.