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: add bufferAsync methods #1145

Merged
merged 7 commits into from May 18, 2021
Merged

feat: add bufferAsync methods #1145

merged 7 commits into from May 18, 2021

Conversation

olavloite
Copy link
Contributor

@olavloite olavloite commented May 7, 2021

Adds bufferAsync methods to TransactionContext. The existing buffer methods were already non-blocking, but the async versions also return an ApiFuture, which make them easier to use when chaining multiple async calls together.

Also changes some calls in the AsyncTransactionManagerTest to use lambdas instead of the test helper methods.

Fixes #1126

Adds bufferAsync methods to TransactionContext. The existing buffer methods
were already non-blocking, but the async versions also return an ApiFuture,
which make them easier to use when chaining multiple async calls together.

Also changes some calls in the AsyncTransactionManagerTest to use lambdas
instead of the test helper methods.

Fixes #1126
@olavloite olavloite requested a review from thiagotnunes May 7, 2021
@olavloite olavloite requested review from as code owners May 7, 2021
@product-auto-label product-auto-label bot added the api: spanner label May 7, 2021
@google-cla google-cla bot added the cla: yes label May 7, 2021
@codecov
Copy link

@codecov codecov bot commented May 7, 2021

Codecov Report

Merging #1145 (f71b96e) into master (cd45643) will decrease coverage by 0.07%.
The diff coverage is 92.59%.

Current head f71b96e differs from pull request most recent head 0f1a5bc. Consider uploading reports for the commit 0f1a5bc to get more accurate results
Impacted file tree graph

@@             Coverage Diff              @@
##             master    #1145      +/-   ##
============================================
- Coverage     84.86%   84.78%   -0.08%     
  Complexity     2762     2762              
============================================
  Files           156      157       +1     
  Lines         14318    14326       +8     
  Branches       1377     1380       +3     
============================================
- Hits          12151    12147       -4     
- Misses         1596     1609      +13     
+ Partials        571      570       -1     
Impacted Files Coverage Δ Complexity Δ
...om/google/cloud/spanner/TransactionRunnerImpl.java 84.61% <91.30%> (-1.34%) 17.00 <0.00> (ø)
...ain/java/com/google/cloud/spanner/SessionPool.java 88.77% <100.00%> (+0.22%) 81.00 <0.00> (ø)
...a/com/google/cloud/spanner/TransactionContext.java 100.00% <100.00%> (ø) 2.00 <2.00> (?)
...ud/spanner/SessionPoolAsyncTransactionManager.java 77.68% <0.00%> (-6.62%) 16.00% <0.00%> (-3.00%)
.../google/cloud/spanner/AbstractLazyInitializer.java 100.00% <0.00%> (+7.14%) 5.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 0b1a4f0...0f1a5bc. Read the comment docs.

// Normally, we would call the async method from the sync method, but this is also safe as
// both are non-blocking anyways, and this prevents the creation of an ApiFuture that is not
// really used when the sync method is called.
buffer(mutation);
Copy link
Contributor

@thiagotnunes thiagotnunes May 10, 2021

Choose a reason for hiding this comment

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

I do have a worry here: it seems we acquire a lock in the buffer method, so that could potentially have some waiting which is not expected by the user.

How big of a problem do you think this is?

Copy link
Contributor Author

@olavloite olavloite May 11, 2021

Choose a reason for hiding this comment

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

That's a good point. We actually take this lock in a couple of the other async methods as well already, so in that sense this does not deviate from the existing methods. The lock is only held for a short period of time in all cases, as it is only held while executing local operations. So in that sense I don't think it is a big problem. Still, we should preferably not take any locks in a method that is marked as non-blocking, as there could in theory be some waiting, albeit short.

I'll have a look and see if I can fix this for all the async methods in this class.

Copy link
Contributor Author

@olavloite olavloite May 17, 2021

Choose a reason for hiding this comment

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

I've not been able to come up with a solution without any locking, but I've reduced it to an absolute minimum by introducing a separate lock that is only for ensuring that mutations that are buffered are either included in the commit, or will throw an exception if commit has already been called. It is not perfect, but the locking should be absolutely minimal as the lock is only held for a couple of simple statements in all cases.

@olavloite olavloite added the do not merge label May 11, 2021
@olavloite olavloite removed the do not merge label May 17, 2021
Copy link
Contributor

@thiagotnunes thiagotnunes left a comment

Thanks for the changes @olavloite

@olavloite olavloite merged commit 7d6816f into master May 18, 2021
16 checks passed
@olavloite olavloite deleted the buffer-async branch May 18, 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.

2 participants