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: AsyncTransactionManager should rollback on close #505

Merged
merged 3 commits into from Oct 14, 2020
Merged

Conversation

olavloite
Copy link
Contributor

@olavloite olavloite commented Oct 9, 2020

AsyncTransctionManager should rollback the transaction if close is called while the transaction is still in state STARTED. Failing to do so, will keep the transaction open on the backend for longer than necessary, holding on to locks until it is garbage collected after 10 seconds.

Fixes #504

AsyncTransctionManager should rollback the transaction if close is called
while the transaction is still in state STARTED. Failing to do so, will
keep the transaction open on the backend for longer than necessary, holding
on to locks until it is garbage collected after 10 seconds.

Fixes #504
@olavloite olavloite requested review from thiagotnunes and elefeint Oct 9, 2020
@olavloite olavloite requested a review from as a code owner Oct 9, 2020
@google-cla google-cla bot added the cla: yes label Oct 9, 2020
@codecov
Copy link

@codecov codecov bot commented Oct 9, 2020

Codecov Report

Merging #505 into master will decrease coverage by 0.02%.
The diff coverage is 81.81%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #505      +/-   ##
============================================
- Coverage     84.03%   84.00%   -0.03%     
+ Complexity     2539     2538       -1     
============================================
  Files           140      140              
  Lines         13871    13889      +18     
  Branches       1328     1329       +1     
============================================
+ Hits          11656    11668      +12     
- Misses         1672     1679       +7     
+ Partials        543      542       -1     
Impacted Files Coverage Δ Complexity Δ
...gle/cloud/spanner/AsyncTransactionManagerImpl.java 70.31% <66.66%> (-5.96%) 12.00 <2.00> (ø)
...ud/spanner/SessionPoolAsyncTransactionManager.java 84.90% <87.50%> (-2.20%) 9.00 <2.00> (+1.00) ⬇️
...ain/java/com/google/cloud/spanner/SessionPool.java 86.87% <0.00%> (+0.17%) 110.00% <0.00%> (ø%)
...ain/java/com/google/cloud/spanner/SessionImpl.java 87.27% <0.00%> (+0.60%) 31.00% <0.00%> (+1.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 3ab7348...0614cc4. Read the comment docs.

Copy link
Contributor

@elefeint elefeint left a comment

Thank you for getting to it so fast!

@@ -54,6 +54,9 @@ public void setSpan(Span span) {

@Override
public void close() {
if (txnState == TransactionState.STARTED) {
rollbackAsync();
Copy link
Contributor

@elefeint elefeint Oct 9, 2020

Choose a reason for hiding this comment

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

Can there be a version of close() that propagates the ApiFutrue from rollbackAsync() back to the caller? Otherwise the application could shut down (or, in my case, the test terminate) before the rollback happens.

Copy link
Contributor Author

@olavloite olavloite Oct 13, 2020

Choose a reason for hiding this comment

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

I've added a closeAsync() method to the AsyncTransactionManager interface that returns an ApiFuture that is done when everything has been rolled back and released. PTAL.

@product-auto-label product-auto-label bot added the api: spanner label Oct 10, 2020
@olavloite olavloite requested a review from as a code owner Oct 13, 2020
Copy link
Contributor

@elefeint elefeint left a comment

Thank you!

@olavloite olavloite merged commit c580df8 into master Oct 14, 2020
17 of 19 checks passed
@olavloite olavloite deleted the issue-504 branch Oct 14, 2020
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.

2 participants