Skip to content
This repository was archived by the owner on Apr 7, 2026. It is now read-only.

feat: add support for Optimistic Concurrency Control#1957

Closed
ko3a4ok wants to merge 0 commit intogoogleapis:mainfrom
ko3a4ok:main
Closed

feat: add support for Optimistic Concurrency Control#1957
ko3a4ok wants to merge 0 commit intogoogleapis:mainfrom
ko3a4ok:main

Conversation

@ko3a4ok
Copy link
Copy Markdown
Contributor

@ko3a4ok ko3a4ok commented Jul 21, 2022

No description provided.

@ko3a4ok ko3a4ok requested a review from a team July 21, 2022 04:53
@product-auto-label product-auto-label Bot added the size: l Pull request size is large. label Jul 21, 2022
@generated-files-bot
Copy link
Copy Markdown

generated-files-bot Bot commented Jul 21, 2022

Warning: This pull request is touching the following templated files:

  • proto-google-cloud-spanner-v1/src/main/java/com/google/spanner/v1/TransactionOptions.java
  • proto-google-cloud-spanner-v1/src/main/java/com/google/spanner/v1/TransactionProto.java

@product-auto-label product-auto-label Bot added the api: spanner Issues related to the googleapis/java-spanner API. label Jul 21, 2022
@ko3a4ok
Copy link
Copy Markdown
Contributor Author

ko3a4ok commented Jul 21, 2022

@ansh0l @rajatbhatta could you please help with review?

rajatbhatta
rajatbhatta suggested changes Jul 21, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@rajatbhatta rajatbhatta left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall, this looks good to me, apart from a few suggestions. Please take a look.

Comment thread google-cloud-spanner/src/main/java/com/google/cloud/spanner/Options.java Outdated
Comment thread google-cloud-spanner/src/main/java/com/google/cloud/spanner/SessionImpl.java Outdated
Comment thread google-cloud-spanner/src/main/java/com/google/cloud/spanner/SessionImpl.java Outdated
Comment thread google-cloud-spanner/src/main/java/com/google/cloud/spanner/TransactionRunnerImpl.java Outdated
rajatbhatta
rajatbhatta reviewed Jul 21, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@rajatbhatta rajatbhatta left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Proto changes should be merged as part of autogenerated PRs (once the proto file changes are made public). Are the corresponding proto file changes public yet?

@ko3a4ok
Copy link
Copy Markdown
Contributor Author

ko3a4ok commented Jul 21, 2022

@rajatbhatta, Proto changes are not in public and behind visibility restriction: TRUSTED_TESTER_OCC. The feature isn't ready on the server side yet, but API has been defined. I wanted to update a client so I can test the feature end-end. So what should I do with protos?

@ko3a4ok ko3a4ok requested a review from rajatbhatta July 22, 2022 04:15
@rajatbhatta
Copy link
Copy Markdown
Contributor

rajatbhatta commented Jul 22, 2022

@rajatbhatta, Proto changes are not in public and behind visibility restriction: TRUSTED_TESTER_OCC. The feature isn't ready on the server side yet, but API has been defined. I wanted to update a client so I can test the feature end-end. So what should I do with protos?

While merging this PR, we'll have to remove the protos. Which means that we should merge this PR only after visibility restriction is lifted, and proto changes are merged and made public (as part of autogenerated PR).

@ansh0l ansh0l requested a review from olavloite July 22, 2022 14:43
olavloite
olavloite approved these changes Jul 22, 2022
View reviewed changes
Copy link
Copy Markdown
Collaborator

@olavloite olavloite left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks generally good to me (with some nits on assertions).

Also; would you mind adding a test to this file:

java-spanner/google-cloud-spanner/src/test/java/com/google/cloud/spanner/RetryOnInvalidatedSessionTest.java

Line 519 in f01d263

public void readWriteTransactionRead() throws InterruptedException {

Those tests verify that everything works as expected if a transaction fails in case of a SessionNotFoundException. That can happen if a session has been garbage collected by the backend with the client knowing/expecting it.

Comment thread google-cloud-spanner/src/main/java/com/google/cloud/spanner/Options.java Outdated
Comment thread ...-spanner/src/test/java/com/google/cloud/spanner/ReadWriteTransactionWithInlineBeginTest.java Outdated
Comment thread ...-spanner/src/test/java/com/google/cloud/spanner/ReadWriteTransactionWithInlineBeginTest.java Outdated
@ansh0l
Copy link
Copy Markdown
Contributor

ansh0l commented Aug 30, 2022

cc @surbhigarg92

@product-auto-label product-auto-label Bot added size: m Pull request size is medium. and removed size: l Pull request size is large. labels Nov 2, 2022
@ko3a4ok ko3a4ok closed this Nov 2, 2022
@product-auto-label product-auto-label Bot added size: u Pull request is empty. and removed size: m Pull request size is medium. labels Nov 2, 2022
@ko3a4ok ko3a4ok mentioned this pull request Nov 3, 2022
Merged
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@rajatbhatta rajatbhatta Awaiting requested review from rajatbhatta

1 more reviewer

@olavloite olavloite olavloite approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

api: spanner Issues related to the googleapis/java-spanner API. size: u Pull request is empty.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@ko3a4ok @rajatbhatta @ansh0l @olavloite