Skip to content
This repository has been archived by the owner on Dec 8, 2021. It is now read-only.

feat: commit begins transaction if not already begun #1143

Merged
merged 4 commits into from
Dec 13, 2019

Conversation

devjgm
Copy link
Contributor

@devjgm devjgm commented Dec 13, 2019

Fixes #689


This change is Reviewable

@devjgm devjgm requested review from mr-salty and devbww December 13, 2019 02:28
@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Dec 13, 2019
@codecov
Copy link

codecov bot commented Dec 13, 2019

Codecov Report

Merging #1143 into master will increase coverage by 0.02%.
The diff coverage is 94.87%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #1143      +/-   ##
=========================================
+ Coverage   93.58%   93.6%   +0.02%     
=========================================
  Files         169     170       +1     
  Lines       13065   13110      +45     
=========================================
+ Hits        12227   12272      +45     
  Misses        838     838
Impacted Files Coverage Δ
google/cloud/spanner/internal/connection_impl.cc 97% <100%> (+0.29%) ⬆️
...gle/cloud/spanner/internal/connection_impl_test.cc 98.82% <93.65%> (+0.03%) ⬆️
...loud/spanner/internal/partial_result_set_resume.cc 90.9% <0%> (-4.55%) ⬇️
...ud/spanner/integration_tests/client_stress_test.cc 82.45% <0%> (-0.88%) ⬇️
google/cloud/spanner/transaction.h 100% <0%> (ø) ⬆️
google/cloud/spanner/database_admin_connection.h 100% <0%> (ø)
.../spanner/benchmarks/multiple_rows_cpu_benchmark.cc 64.05% <0%> (+0.03%) ⬆️
google/cloud/spanner/internal/spanner_stub.cc 68.13% <0%> (+1.09%) ⬆️

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 fb5edf1...4da2699. Read the comment docs.

if (s.selector_case() != spanner_proto::TransactionSelector::kId) {
spanner_proto::BeginTransactionRequest begin;
begin.set_session(session->session_name());
*begin.mutable_options() = s.begin();
Copy link
Contributor

Choose a reason for hiding this comment

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

Even though it doesn't make sense to pass a single-use/read-only transaction to Commit(), it also doesn't make sense to pass any read-only transaction, yet we support the latter and not the former. So, we might as well look at s.has_single_use() and use its TransactionOptions, rather than using the default TransactionOptions, and let the chips fall where they may.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wouldn't say that we "support" read-only transactions. Any caller who passes in a read-only transaction to Commit() will get an error. I believe that's true before and after this PR.

To call Commit() correctly, callers must pass in a read-write transaction. RW transactions have all their options in the "begin" field of the selector, so that's why I think we only need to copy that field from the selector into the mutable_options in the request.

I don't think we should write extra code to deal with things like single-use RO transactions when callers are not allowed to pass them, and if they did, it's guaranteed to fail.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Discussed this offline. I changed the code to consider the case where the input selector was a single_use transaction as @devbww was suggesting. We know this will give an error, but it's what the user asked for.

google/cloud/spanner/internal/connection_impl_test.cc Outdated Show resolved Hide resolved
@devjgm devjgm merged commit 4628cd1 into googleapis:master Dec 13, 2019
@devjgm devjgm deleted the commit-fix branch December 13, 2019 17:56
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Resolve use of single_use_transaction in Commit
3 participants