feat: add bufferAsync methods #1145
Conversation
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
product-auto-label
bot
added
the
api: spanner
google-cla
bot
added
the
cla: yes
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
| // 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); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
added
the
do not merge
olavloite
removed
the
do not merge
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