feat(spanner): support request and transaction tags #3233

olavloite · 2020-11-19T09:11:19Z

Adds support for request and transaction tags. This change depends on proto changes that have not yet been merged.

syeduguri · 2020-11-20T06:29:28Z

spanner/client_test.go

+	}
+}
+
+func checkRequestsForTags(t *testing.T, server InMemSpannerServer, reqCount int, qo QueryOptions) {


Better to rename it as "expectedQueryOptions".
Same comment for next function also.

I don't think QueryOptions is the right name in this case, as QueryOptions also contain other options than tags, and those are not checked here. I think checkFor...RequestOptions would fit better here.

syeduguri · 2020-11-20T06:38:54Z

spanner/transaction.go

+	// The transaction tag to use for a read/write transaction.
+	// This tag is automatically included with each statement and the commit
+	// request of a read/write transaction.
+	TransactionTag string


Just wondering why there was empty TransactionOptions struct before this change.

We had a major version bump recently, and added a number of breaking changes already back then that we knew would be needed for (among others) tagging support. Adding a struct is not breaking, but we also added a number of new methods that use this struct as a parameter. That way we don't need to have two major version bumps in a short period of time.

syeduguri · 2020-11-20T06:46:14Z

spanner/transaction.go

@@ -151,6 +160,10 @@ func (t *txReadOnly) ReadWithOptions(ctx context.Context, table string, keys Key
 					KeySet:      kset,
 					ResumeToken: resumeToken,
 					Limit:       int64(limit),
+					RequestOptions: &sppb.RequestOptions{
+						RequestTag:     requestTag,
+						TransactionTag: t.qo.transactionTag,


We don't need transactionTag for readonly transactions.

That is true, but this method is used for both read-only and read/write transactions. The transactionTag field can however only be set for read/write transactions, so it will always be empty when this is used in combination with a read-only transaction.

syeduguri

Looks like changes covered all APIs and scenarios. Just want to know where we handled PartitionedDML requests.
Left few minor comments.

olavloite · 2020-11-20T13:24:24Z

Looks like changes covered all APIs and scenarios. Just want to know where we handled PartitionedDML requests.
Left few minor comments.

@syeduguri Thanks for the review.
Good point regarding PDML. That was missing but has been added now. PTAL.

syeduguri · 2020-11-20T22:30:09Z

Changes looks good to me.

hengfengli · 2021-03-30T01:53:11Z

spanner/client_test.go

+	}
+}
+
+func TestClient_Apply_Priority(t *testing.T) {


Apply_Tagging?

hengfengli · 2021-03-30T01:56:20Z

spanner/transaction.go

+	RequestTag string
+	// transactionTag is unexported as applications should set this on the transaction and
+	// the transaction will then set this field for each request during the transaction.
+	transactionTag string


Why don't we put this to TransactionOptions? transactionTag seems to be more tight with a transaction instead of a query/update?

That's a reasonable question. I'll look into that to see whether that would be more reasonable once the proto changes have been merged. I tried to re-generate locally, but that seems to fail at the moment.

I've removed the transactionTag from the QueryOptions as it was not a logical place to keep it.

hengfengli · 2021-03-30T01:58:21Z

spanner/transaction.go

@@ -874,6 +922,10 @@ func (t *ReadWriteTransaction) BatchUpdate(ctx context.Context, stmts []Statemen
 		Transaction: ts,
 		Statements:  sppbStmts,
 		Seqno:       atomic.AddInt64(&t.sequenceNumber, 1),
+		RequestOptions: &sppb.RequestOptions{
+			RequestTag:     opts.RequestTag,
+			TransactionTag: t.qo.transactionTag,


What I mean is that we extract TransactionTag from t.txOpts (transaction options) instead of t.qo (query options)? Why do we need to keep transactionTag in QueryOptions? Just a thought.

Good point, done (see also above).

hengfengli

LGTM

skuruppu · 2021-04-22T09:10:47Z

@olavloite unfortunately we have to roll this back since we are not able to release it yet as the backend is not ready for this. Hope you don't mind.

FYI @hengfengli

olavloite · 2021-04-22T09:36:23Z

@olavloite unfortunately we have to roll this back since we are not able to release it yet as the backend is not ready for this. Hope you don't mind.

FYI @hengfengli

I'll prepare a revert. We probably also need to revert (or tweak) #3905 as well.

UPDATE: It seems that this PR cannot be (automatically) reverted because of other changes. I think that that is caused by the changes in #3905, which means that we need to revert that first. Then we can try to automatically revert this.

There's a revert PR for #3905 here: #3987

This reverts commit 2b416e8.

BREAKING CHANGE: This removes the following exported functions and fields: - TransactionTag(tag string) ApplyOption - TransactionOptions.TransactionTag - QueryOption.RequestTag - ReadOption.RequestTag This reverts commit 2b416e8.

…#3989)" This reverts commit a18f1bc.

…#3989)" (#4336) This reverts commit a18f1bc.

feat: support request and transaction tags

09d32e0

Adds support for request and transaction tags. This change depends on proto changes that have not yet been merged.

olavloite added the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Nov 19, 2020

olavloite requested review from hengfengli and skuruppu November 19, 2020 09:11

olavloite requested a review from a team as a code owner November 19, 2020 09:11

google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Nov 19, 2020

tbpg changed the title ~~feat: support request and transaction tags~~ feat(spanner): support request and transaction tags Nov 19, 2020

tbpg added the api: spanner Issues related to the Spanner API. label Nov 19, 2020

skuruppu requested a review from syeduguri November 19, 2020 22:57

syeduguri reviewed Nov 20, 2020

View reviewed changes

fix: add tags for PDML

adc473a

olavloite requested a review from syeduguri November 20, 2020 16:05

syeduguri approved these changes Nov 20, 2020

View reviewed changes

olavloite added 2 commits December 3, 2020 17:18

Merge branch 'master' into spanner-tagging

de128ca

feat: support tags in Apply

16eb442

hengfengli reviewed Mar 30, 2021

View reviewed changes

olavloite added 3 commits March 30, 2021 11:26

Merge branch 'master' into spanner-tagging

415eb70

Merge branch 'master' into spanner-tagging

6f40e0d

cleanup: remove unused code and undo unnecessary changes from merge

0071dee

olavloite removed the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Apr 1, 2021

Merge branch 'master' into spanner-tagging

822b0cc

olavloite requested a review from hengfengli April 1, 2021 13:56

hengfengli approved these changes Apr 6, 2021

View reviewed changes

skuruppu approved these changes Apr 6, 2021

View reviewed changes

Merge branch 'master' into spanner-tagging

ae3e4b5

olavloite added the automerge Merge the pull request once unit tests and other checks pass. label Apr 6, 2021

gcf-merge-on-green bot merged commit 2b416e8 into master Apr 6, 2021

gcf-merge-on-green bot deleted the spanner-tagging branch April 6, 2021 07:54

gcf-merge-on-green bot removed the automerge Merge the pull request once unit tests and other checks pass. label Apr 6, 2021

olavloite added a commit that referenced this pull request Apr 22, 2021

Revert "feat(spanner): support request and transaction tags (#3233)"

e6ea5c5

This reverts commit 2b416e8.

olavloite mentioned this pull request Apr 22, 2021

Revert "feat(spanner): support request and transaction tags" #3989

Merged

apstndb mentioned this pull request Jun 7, 2021

google-cloud-go/spanner のバージョン間差分リーディングスレ gcpug/nouhau#127

Open

olavloite added a commit that referenced this pull request Jun 29, 2021

Revert "revert(spanner): support request and transaction tags (#3233) (…

6a1f314

…#3989)" This reverts commit a18f1bc.

olavloite mentioned this pull request Jun 29, 2021

feat(spanner): support request and transaction tags #4336

Merged

olavloite added a commit that referenced this pull request Jun 29, 2021

Revert "revert(spanner): support request and transaction tags (#3233) (…

f08c73a

…#3989)" (#4336) This reverts commit a18f1bc.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(spanner): support request and transaction tags #3233

feat(spanner): support request and transaction tags #3233

olavloite commented Nov 19, 2020

syeduguri Nov 20, 2020 •

edited

olavloite Nov 20, 2020

syeduguri Nov 20, 2020

olavloite Nov 20, 2020 •

edited

syeduguri Nov 20, 2020

olavloite Nov 20, 2020

syeduguri left a comment

olavloite commented Nov 20, 2020

syeduguri commented Nov 20, 2020

hengfengli Mar 30, 2021

olavloite Mar 30, 2021

hengfengli Mar 30, 2021

olavloite Mar 30, 2021

olavloite Apr 1, 2021

hengfengli Mar 30, 2021 •

edited

olavloite Apr 1, 2021

hengfengli left a comment

skuruppu commented Apr 22, 2021 •

edited

olavloite commented Apr 22, 2021 •

edited

feat(spanner): support request and transaction tags #3233

feat(spanner): support request and transaction tags #3233

Conversation

olavloite commented Nov 19, 2020

syeduguri Nov 20, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

olavloite Nov 20, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

syeduguri left a comment

Choose a reason for hiding this comment

olavloite commented Nov 20, 2020

syeduguri commented Nov 20, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hengfengli Mar 30, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hengfengli left a comment

Choose a reason for hiding this comment

skuruppu commented Apr 22, 2021 • edited

olavloite commented Apr 22, 2021 • edited

syeduguri Nov 20, 2020 •

edited

olavloite Nov 20, 2020 •

edited

hengfengli Mar 30, 2021 •

edited

skuruppu commented Apr 22, 2021 •

edited

olavloite commented Apr 22, 2021 •

edited