Conversation
olavloite
added
the
do not merge
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces performance optimizations and refactoring across the google-cloud-spanner package, including background stream draining to consume trailing gRPC metadata asynchronously, optimized row decoding, and cached metric attributes. However, the review identifies critical regressions: removing the deletion of old multiplexed sessions during rotation and closure in both the synchronous and asynchronous database session managers introduces backend session resource leaks. Additionally, removing tracing and metrics capture from Session.run_in_transaction causes an observability regression for direct session usage.
| async with self._multiplexed_session_lock: | ||
| old_session = self._multiplexed_session | ||
| self._multiplexed_session = new_session | ||
|
|
||
| if old_session is not None: | ||
| try: | ||
| await CrossSync.run_if_async(old_session.delete) | ||
| except Exception: | ||
| pass | ||
|
|
||
| return True |
There was a problem hiding this comment.
Removing the deletion of the old multiplexed session during rotation (old_session.delete()) introduces a resource leak on the Spanner backend. Although multiplexed sessions are designed to be shared, they are still active session resources on the server. Failing to delete them when they are rotated out means they will remain active until they idle-expire (which can take up to an hour), potentially leading to exceeding the session limit on the database. We should restore the deletion of the old session.
async with self._multiplexed_session_lock:
old_session = self._multiplexed_session
self._multiplexed_session = new_session
if old_session is not None:
try:
await CrossSync.run_if_async(old_session.delete)
except Exception:
pass
return True| session_to_delete = self._multiplexed_session | ||
| self._multiplexed_session = None | ||
| await session_to_delete.delete() | ||
| self._multiplexed_session = None |
There was a problem hiding this comment.
Failing to delete the multiplexed session when closing the session manager leaks the session resource on the Spanner backend. We should restore the deletion of self._multiplexed_session during close().
| self._multiplexed_session = None | |
| if self._multiplexed_session is not None: | |
| session_to_delete = self._multiplexed_session | |
| self._multiplexed_session = None | |
| await session_to_delete.delete() |
| with self._multiplexed_session_lock: | ||
| old_session = self._multiplexed_session | ||
| self._multiplexed_session = new_session | ||
|
|
||
| if old_session is not None: | ||
| try: | ||
| CrossSync._Sync_Impl.run_if_async(old_session.delete) | ||
| except Exception: | ||
| pass | ||
|
|
||
| return True |
There was a problem hiding this comment.
Removing the deletion of the old multiplexed session during rotation (old_session.delete()) introduces a resource leak on the Spanner backend. Although multiplexed sessions are designed to be shared, they are still active session resources on the server. Failing to delete them when they are rotated out means they will remain active until they idle-expire (which can take up to an hour), potentially leading to exceeding the session limit on the database. We should restore the deletion of the old session.
with self._multiplexed_session_lock:
old_session = self._multiplexed_session
self._multiplexed_session = new_session
if old_session is not None:
try:
CrossSync._Sync_Impl.run_if_async(old_session.delete)
except Exception:
pass
return True| session_to_delete = self._multiplexed_session | ||
| self._multiplexed_session = None | ||
| session_to_delete.delete() | ||
| self._multiplexed_session = None |
There was a problem hiding this comment.
Failing to delete the multiplexed session when closing the session manager leaks the session resource on the Spanner backend. We should restore the deletion of self._multiplexed_session during close().
| self._multiplexed_session = None | |
| if self._multiplexed_session is not None: | |
| session_to_delete = self._multiplexed_session | |
| self._multiplexed_session = None | |
| session_to_delete.delete() |
| txn._multiplexed_session_previous_transaction_id = ( | ||
| previous_transaction_id | ||
| ) | ||
| span = get_current_span() |
There was a problem hiding this comment.
Removing trace_call and MetricsCapture from Session.run_in_transaction is a regression in observability. Session is a public class, and run_in_transaction is a public API. If a user calls session.run_in_transaction directly, they will no longer get any tracing spans or metrics captured for the transaction. While this might have been done as a micro-optimization to reduce span count when called via Database.run_in_transaction, it breaks observability for direct Session usage. We should keep the span and metrics capture, or at least ensure they are active when there is no active parent span.
| span, | ||
| "Transaction.commit failed due to GoogleAPICallError, not retrying", | ||
| span_attributes, | ||
| span = get_current_span() |
There was a problem hiding this comment.
Removing trace_call and MetricsCapture from Session.run_in_transaction is a regression in observability. Session is a public class, and run_in_transaction is a public API. If a user calls session.run_in_transaction directly, they will no longer get any tracing spans or metrics captured for the transaction. While this might have been done as a micro-optimization to reduce span count when called via Database.run_in_transaction, it breaks observability for direct Session usage. We should keep the span and metrics capture, or at least ensure they are active when there is no active parent span.
1 participant
Do not merge: Prototype