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

fix: safeguard against statement errors when requesting a transaction #800

Merged
merged 1 commit into from Jan 14, 2021

Conversation

olavloite
Copy link
Contributor

@olavloite olavloite commented Jan 14, 2021

Adds a number of safeguards against hanging transactions if the first statement of a transaction for whatever reason fails to return a transaction ID, or otherwise returns an unexpected error. This PR contains the following safeguards:

  1. If a statement requested a new transaction from Cloud Spanner and receives a response without a transaction ID, the statement will throw an exception. This should theoretically never happen. If it did happen, it would cause the second statement in the transaction to be 'stuck', as it would wait indefinitely for the first statement to return a transaction.
  2. If the first statement of a transaction throws an unexpected error (a Throwable that is not a SpannerException), the exception will be translated to a SpannerException and handled as such. This error will mark the transaction as unusable, as the first statement failed to return a transaction.
  3. If the first statement fails to respond at all (no response, no error), the second statement will wait at most 60 seconds for a transaction ID to be returned, and then fail with a ABORTED error. This will cause the transaction to be retried automatically.

Fixes #799

@olavloite olavloite requested review from thiagotnunes and skuruppu Jan 14, 2021
@olavloite olavloite requested a review from as a code owner Jan 14, 2021
@product-auto-label product-auto-label bot added the api: spanner label Jan 14, 2021
@google-cla google-cla bot added the cla: yes label Jan 14, 2021
@olavloite olavloite added the kokoro:force-run label Jan 14, 2021
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run label Jan 14, 2021
@codecov
Copy link

@codecov codecov bot commented Jan 14, 2021

Codecov Report

Merging #800 (02a886b) into master (1a71e50) will increase coverage by 0.08%.
The diff coverage is 92.85%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #800      +/-   ##
============================================
+ Coverage     85.01%   85.09%   +0.08%     
- Complexity     2562     2564       +2     
============================================
  Files           143      143              
  Lines         14015    14030      +15     
  Branches       1341     1341              
============================================
+ Hits          11915    11939      +24     
+ Misses         1537     1531       -6     
+ Partials        563      560       -3     
Impacted Files Coverage Δ Complexity Δ
...om/google/cloud/spanner/TransactionRunnerImpl.java 85.74% <92.10%> (+0.87%) 9.00 <0.00> (ø)
.../com/google/cloud/spanner/AbstractReadContext.java 87.07% <100.00%> (ø) 48.00 <1.00> (ø)
...va/com/google/cloud/spanner/AbstractResultSet.java 83.30% <100.00%> (+0.03%) 28.00 <0.00> (ø)
...ain/java/com/google/cloud/spanner/SessionPool.java 89.02% <0.00%> (+0.19%) 71.00% <0.00%> (ø%)
...ud/spanner/SessionPoolAsyncTransactionManager.java 87.30% <0.00%> (+1.58%) 13.00% <0.00%> (+2.00%)
...m/google/cloud/spanner/connection/SpannerPool.java 86.11% <0.00%> (+1.66%) 31.00% <0.00%> (ø%)

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 1a71e50...02a886b. Read the comment docs.

@olavloite olavloite changed the title fix: safeguard against statements errors when requesting tx fix: safeguard against statement errors when requesting a transaction Jan 14, 2021
Copy link
Contributor

@thiagotnunes thiagotnunes left a comment

Thanks for this fixes @olavloite, really appreciate it.

@thiagotnunes thiagotnunes merged commit c4776e4 into master Jan 14, 2021
19 checks passed
@thiagotnunes thiagotnunes deleted the issue-799 branch Jan 14, 2021
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.

3 participants