-
Notifications
You must be signed in to change notification settings - Fork 101
feat: default enable multiplex session for all operations unless explicitly set to false #1394
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
Changes from all commits
ced5d4b
4878ff3
15767c9
7225b88
f224acb
311451a
be1c9d8
e5993ff
73f6145
b940b69
679f9be
3ad9ce9
8e8449d
7036734
8741633
67ea777
6f4add1
8976025
63bac62
739a78e
ccf299f
f8d0345
c942b18
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -133,6 +133,8 @@ def _restart_on_unavailable( | |
| # Update the transaction from the response. | ||
| if transaction is not None: | ||
| transaction._update_for_result_set_pb(item) | ||
| if item.precommit_token is not None and transaction is not None: | ||
| transaction._update_for_precommit_token_pb(item.precommit_token) | ||
|
|
||
| if item.resume_token: | ||
| resume_token = item.resume_token | ||
|
|
@@ -1013,9 +1015,6 @@ def _update_for_result_set_pb( | |
| if result_set_pb.metadata and result_set_pb.metadata.transaction: | ||
| self._update_for_transaction_pb(result_set_pb.metadata.transaction) | ||
|
|
||
| if result_set_pb.precommit_token: | ||
| self._update_for_precommit_token_pb(result_set_pb.precommit_token) | ||
|
|
||
| def _update_for_transaction_pb(self, transaction_pb: Transaction) -> None: | ||
| """Updates the snapshot for the given transaction. | ||
|
|
||
|
|
@@ -1031,7 +1030,7 @@ def _update_for_transaction_pb(self, transaction_pb: Transaction) -> None: | |
| self._transaction_id = transaction_pb.id | ||
|
|
||
| if transaction_pb.precommit_token: | ||
| self._update_for_precommit_token_pb(transaction_pb.precommit_token) | ||
| self._update_for_precommit_token_pb_unsafe(transaction_pb.precommit_token) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. any reason why we need two methods?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. there was a deadlock happening, transaction was locked already when calling _update_for_transaction_pb, and in _update_for_precommit_token_pb we were again trying to lock it. This was the issue from previous release which got uncovered once I tried making mux default enabled.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Read this comment And then when we were calling _update_for_precommit_token_pb we were taking the lock again So we needed 2 methods which takes lock when updating precommit token from unlocked code, and one which is called from places which assume lock is already taken |
||
|
|
||
| def _update_for_precommit_token_pb( | ||
| self, precommit_token_pb: MultiplexedSessionPrecommitToken | ||
|
|
@@ -1044,10 +1043,22 @@ def _update_for_precommit_token_pb( | |
| # Because multiple threads can be used to perform operations within a | ||
| # transaction, we need to use a lock when updating the precommit token. | ||
| with self._lock: | ||
| if self._precommit_token is None or ( | ||
| precommit_token_pb.seq_num > self._precommit_token.seq_num | ||
| ): | ||
| self._precommit_token = precommit_token_pb | ||
| self._update_for_precommit_token_pb_unsafe(precommit_token_pb) | ||
|
|
||
| def _update_for_precommit_token_pb_unsafe( | ||
| self, precommit_token_pb: MultiplexedSessionPrecommitToken | ||
| ) -> None: | ||
| """Updates the snapshot for the given multiplexed session precommit token. | ||
| This method is unsafe because it does not acquire a lock before updating | ||
| the precommit token. It should only be used when the caller has already | ||
| acquired the lock. | ||
| :type precommit_token_pb: :class:`~google.cloud.spanner_v1.MultiplexedSessionPrecommitToken` | ||
| :param precommit_token_pb: The multiplexed session precommit token to update the snapshot with. | ||
| """ | ||
| if self._precommit_token is None or ( | ||
| precommit_token_pb.seq_num > self._precommit_token.seq_num | ||
| ): | ||
| self._precommit_token = precommit_token_pb | ||
|
|
||
|
|
||
| class Snapshot(_SnapshotBase): | ||
|
|
||
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.
this is because in followup PR we will be removing 3.8 runtime support
#1395