Skip to content
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: introduce TransactionOptions and UpdateOptions #716

Merged
merged 2 commits into from Dec 15, 2020

Conversation

olavloite
Copy link
Contributor

@olavloite olavloite commented Dec 11, 2020

Adds TransactionOptions and UpdateOptions for read/write transactions and statements. These can be used in the future to specify options to affect how transactions and statements will be executed.

These changes can be used as the basis for #676 and #576 to ensure both changes use the same approach, and to prevent enormous merge conflicts.

Adds TransactionOptions and UpdateOptions for read/write transactions and statements.
These can be used in the future to specify options to affect how transactions and
statements will be executed.
@olavloite olavloite requested review from as code owners Dec 11, 2020
@google-cla google-cla bot added the cla: yes label Dec 11, 2020
@product-auto-label product-auto-label bot added the api: spanner label Dec 11, 2020
Copy link
Member

@mayurkale22 mayurkale22 left a comment

I like this PR, this will certainly simplify #676 and #576. Thank you so much for doing this!

@@ -340,10 +343,11 @@ public void run() {
return res;
}

TransactionContextImpl newTransaction() {
TransactionContextImpl newTransaction(Options options) {
Copy link
Member

@mayurkale22 mayurkale22 Dec 11, 2020

Choose a reason for hiding this comment

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

Can this be just TransactionOption instead of Options?

Copy link
Contributor Author

@olavloite olavloite Dec 11, 2020

Choose a reason for hiding this comment

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

That would be possible (assuming you mean TransactionOption...), but I would prefer to keep this as the Options object. This makes it a required argument, making it less likely to be missed by accident as it will cause a compile error if you don't supply an Options object. If we change it to TransactionOption... it would not cause a compile time error if we forget to pass in the options somewhere.

Copy link
Member

@mayurkale22 mayurkale22 Dec 11, 2020

Choose a reason for hiding this comment

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

Hmm, makes sense.

@mayurkale22 mayurkale22 mentioned this pull request Dec 11, 2020
1 task
@codecov
Copy link

@codecov codecov bot commented Dec 11, 2020

Codecov Report

Merging #716 (ae2cf93) into master (f14f7c9) will increase coverage by 0.08%.
The diff coverage is 88.34%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #716      +/-   ##
============================================
+ Coverage     85.03%   85.12%   +0.08%     
- Complexity     2553     2561       +8     
============================================
  Files           142      142              
  Lines         13930    13952      +22     
  Branches       1326     1331       +5     
============================================
+ Hits          11846    11877      +31     
+ Misses         1527     1515      -12     
- Partials        557      560       +3     
Impacted Files Coverage Δ Complexity Δ
...rc/main/java/com/google/cloud/spanner/Options.java 85.48% <40.00%> (-3.99%) 67.00 <2.00> (+2.00) ⬇️
...oogle/cloud/spanner/PartitionedDmlTransaction.java 82.60% <83.33%> (+0.19%) 15.00 <0.00> (ø)
...ain/java/com/google/cloud/spanner/SessionPool.java 89.02% <91.17%> (+0.48%) 71.00 <0.00> (ø)
...ain/java/com/google/cloud/spanner/SessionImpl.java 85.79% <92.30%> (+1.77%) 30.00 <4.00> (+1.00)
...om/google/cloud/spanner/TransactionRunnerImpl.java 84.36% <93.75%> (+0.09%) 9.00 <0.00> (ø)
.../com/google/cloud/spanner/AbstractReadContext.java 86.76% <100.00%> (ø) 47.00 <0.00> (ø)
...gle/cloud/spanner/AsyncTransactionManagerImpl.java 71.83% <100.00%> (+0.40%) 12.00 <0.00> (ø)
...a/com/google/cloud/spanner/DatabaseClientImpl.java 84.46% <100.00%> (+3.51%) 21.00 <2.00> (+2.00)
...ud/spanner/SessionPoolAsyncTransactionManager.java 85.71% <100.00%> (+0.34%) 11.00 <0.00> (ø)
...m/google/cloud/spanner/TransactionManagerImpl.java 88.88% <100.00%> (+0.20%) 20.00 <1.00> (ø)
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f14f7c9...ae2cf93. Read the comment docs.

Copy link
Contributor

@thiagotnunes thiagotnunes left a comment

LGTM

@olavloite
Copy link
Contributor Author

@olavloite olavloite commented Dec 14, 2020

@skuruppu Do you want to weigh in on this as well? It is technically a (binary) breaking change as we are adding vararg arguments to a number of public interface methods. Existing client application code does not need to be changed, but it breaks binary compatibility, so it needs to be recompiled.

@skuruppu
Copy link
Contributor

@skuruppu skuruppu commented Dec 15, 2020

I think previously, we haven't marked changes that break binary compatibility as breaking changes so this is fine.

@olavloite olavloite merged commit 5c96fab into master Dec 15, 2020
21 checks passed
@olavloite olavloite deleted the introduce-transaction-options branch Dec 15, 2020
thiagotnunes pushed a commit that referenced this issue May 6, 2021
* feat: introduce TransactionOptions and UpdateOptions

Adds TransactionOptions and UpdateOptions for read/write transactions and statements.
These can be used in the future to specify options to affect how transactions and
statements will be executed.

* test: add options tests
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: spanner cla: yes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants