Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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: support aborted transactions internal retry #544
feat: support aborted transactions internal retry #544
Changes from 5 commits
50580b4
481db2a
313e16d
3a7537c
4edf6c1
f87129a
49e17be
a8158b3
ccf5385
a537628
0b1a641
0e2ca3e
a4ffab5
fc890c2
d204836
9ea2a01
7e70d86
870e170
450b91b
578eaa2
59d597a
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this will be a problem. Assuming that this is using the
ExecuteStreamingSql
RPC, then eachnext()
call could potentially mean that a new RPC is executed. So for the sake of simplicity, assume in the example below that each call tonext()
executes theExecuteStreamingSql
RPC.Assume the following situation:
SELECT LastName FROM Singers ORDER BY LastName
in transaction 1.fetchone()
which returns 'Allison'.fetchone()
again. This should return 'Morrison', but as it needs to callExecuteStreamingSql
it will (probably) use the transaction id of the original transaction (unless that transaction id has somehow been replaced in the underlying iterator). If it does use the old transaction id, the RPC will fail with yet anotherAborted
error, and that will repeat itself until the transaction retry limit has been reached.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems to me we can just drop the
_transaction
property, so thatConnection
will initiate a new one on the nextexecute()
call.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@IlyaFaer @c24t
Sorry for reopening this, and this comment should not be considered blocking for merging this PR, but I think we need to look into this once more. Only dropping the
_transaction
property will in this case not be enough for the following reason:executeSql
is called, a streaming iterator is returned to the application.The JDBC driver client solves the above problem by wrapping all streaming iterators before returning these to the client application. That makes it possible for the JDBC driver to replace the underlying streaming iterator with a new one when a transaction has been aborted and successfully retried.
We should add that to the Python DBApi as well, but we could do that in a separate PR to prevent this PR from becoming even bigger than it already is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@c24t, @olavloite, hm-m. I think we're protected from errors here, because our connection API doesn't actually give streaming result objects to a user.
Here is where we're getting a streaming iterator:
python-spanner-django/google/cloud/spanner_dbapi/cursor.py
Lines 167 to 170 in 196c449
So, iterator is held in the protected property
_itr
, and users will be streaming it withCursor.fetch*()
methods, without actual access to the iterator itself:python-spanner-django/google/cloud/spanner_dbapi/cursor.py
Lines 204 to 212 in 196c449
Where
next(self)
is callingnext(self._itr)
here:python-spanner-django/google/cloud/spanner_dbapi/cursor.py
Lines 293 to 296 in 196c449
Thus, if a transaction failed, the connection will drop the transaction, checkout a new one, re-run all the statements, each of which will replace
_itr
with a new streamed iterator. So, all the iterators are processed internally, and will be replaced on a retry, as I see.