chore(spanner): support mutation only operation for read-write mux #11342
Conversation
rahul2393
force-pushed
the
mux-rw-mutations
branch
from
6af69d6 to
0cf30f8
Compare
rahul2393
force-pushed
the
mux-rw-mutations
branch
from
0cf30f8 to
0da574c
Compare
| @@ -1660,23 +1665,26 @@ func (co CommitOptions) merge(opts CommitOptions) CommitOptions { | |||
| func (t *ReadWriteTransaction) commit(ctx context.Context, options CommitOptions) (CommitResponse, error) { | |||
| resp := CommitResponse{} | |||
| t.mu.Lock() | |||
| mPb, bmPb, err := mutationsProto(t.wb) | |||
There was a problem hiding this comment.
can we add a comment saying what mPB and bmPB means here?
| @@ -1078,6 +1078,9 @@ func (s *inMemSpannerServer) BeginTransaction(ctx context.Context, req *spannerp | |||
| } | |||
| s.updateSessionLastUseTime(session.Name) | |||
| tx := s.beginTransaction(session, req.Options) | |||
| if session.Multiplexed && req.MutationKey != nil { | |||
| tx.PrecommitToken = s.getPreCommitToken(string(tx.Id), "BeginTransactionPrecommitToken") | |||
There was a problem hiding this comment.
nit: Can we rename this to TransactionPrecommitToken to maintain consistency with Java?
| Insert("t_foo", []string{"col1", "col2"}, []interface{}{int64(1), int64(2)}), | ||
| Update("t_foo", []string{"col1", "col2"}, []interface{}{"one", []byte(nil)}), | ||
| } | ||
| if err := tx.BufferWrite(ms); err != nil { |
There was a problem hiding this comment.
can we verify that the BeginTransactionRequest has a mutation key set and not null?
| {"Only Query", true, false, false, false, "PartialResultSetPrecommitToken", 3}, | ||
| {"Query and Update", true, true, false, false, "ResultSetPrecommitToken", 4}, | ||
| {"Query, Update, and Batch Update", true, true, true, false, "ExecuteBatchDmlResponsePrecommitToken", 5}, | ||
| {"Only Mutations", false, false, false, true, "BeginTransactionPrecommitToken", 1}, |
There was a problem hiding this comment.
Can you add a negative test scenario also?
i.e when a Commit RPC returns aborted then the retry should set mutation key and the commit request should have latest precommit token
https://github.com/harshachinta/java-spanner/blob/01c03144134febfb3c83e1bc80a7533f53f067ba/google-cloud-spanner/src/test/java/com/google/cloud/spanner/MultiplexedSessionDatabaseClientMockServerTest.java#L1140
| @@ -413,19 +413,31 @@ func TestReadWriteTransaction_PrecommitToken(t *testing.T) { | |||
| query bool | |||
| update bool | |||
| batchUpdate bool | |||
| mutationsOnly bool | |||
There was a problem hiding this comment.
Can you add one test case with client.Apply and verify the same behavior?
https://cloud.google.com/spanner/docs/modify-mutation-api#go
| func mutationsProto(ms []*Mutation) ([]*sppb.Mutation, *sppb.Mutation, error) { | ||
| var selectedMutation *Mutation | ||
| var nonInsertMutations []*Mutation | ||
| maxValues := -1 |
There was a problem hiding this comment.
Why do we need to store seperate maxValues variable?
This will always be equal to selectedMutation.values() if selectedMutation is not null ?
The following describes the case when a read-write transaction is executed on a multiplexed session and this PR handles this scenario:
If a read-write transaction contains only mutations, a random mutation is selected from the mutation list and sent with the BeginTransactionRequest. The resulting Transaction response includes a precommit token, which is tracked by the client library and used in the subsequent CommitRequest