refactor(Spanner): Spanner.Data depends on MUX types directly by amanda-tarafa · Pull Request #15535 · googleapis/google-cloud-dotnet

amanda-tarafa · 2026-03-25T21:52:57Z

No description provided.

Note that Spanner.Data already used MUX only, but it did so through the session pool API interface.

… information

gemini-code-assist · 2026-03-25T21:53:39Z

Summary of Changes

Hello, I'm Gemini Code Assist¹! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request undertakes a significant internal refactoring of the Google.Cloud.Spanner.Data library's session and transaction management. The changes introduce a new SessionManager component that centralizes the creation and caching of underlying SpannerClient and ManagedSession objects. This architectural shift allows higher-level components like SpannerConnection and SpannerTransaction to interact more directly with these core types, reducing complexity and paving the way for future performance and feature enhancements by aligning more closely with the underlying client library's direct interaction model.

Highlights

New Session Management Layer: Introduced a new SessionManager class responsible for directly managing and caching SpannerClient and ManagedSession instances, streamlining client and session acquisition.
Direct ManagedTransaction Usage: Refactored SpannerConnection, SpannerTransaction, EphemeralTransaction, and RetriableTransaction to directly utilize ManagedTransaction instead of PooledSession, simplifying transaction lifecycle management.
SessionPoolManager Integration: Updated SessionPoolManager to delegate its core responsibilities to the new SessionManager, indicating a shift in the primary session management abstraction.
Enhanced Transaction Creation Options: Added EffectiveTimestampBound to SpannerTransactionCreationOptions for more robust and centralized timestamp bound resolution, and refined the logic for generating transaction options.

Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature	Command	Description
Code Review	`/gemini review`	Performs a code review for the current pull request in its current state.
Pull Request Summary	`/gemini summary`	Provides a summary of the current pull request in its current state.
Comment	@gemini-code-assist	Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help	`/gemini help`	Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here.

Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩

gemini-code-assist

Code Review

This pull request introduces a new SessionManager to centralize the management of ManagedSession and SpannerClient instances, effectively replacing the previous PooledSession and SessionPoolManager architecture. This significant refactoring impacts how transactions are acquired, used, and disposed across the codebase, including SpannerConnection, SpannerTransaction, EphemeralTransaction, and RetriableTransaction. New tests for SessionManager have been added, and existing tests and logic have been updated to reflect the new session management approach. Review comments highlighted the need to correctly handle ValueTask with Task.Run by converting it to a Task using .AsTask(), and also suggested removing an unused using statement and a line containing only whitespace for code cleanliness.

gemini-code-assist · 2026-03-25T21:55:54Z

-                var reader = request.ExecuteReadOrQueryStreamReader(session, callSettings);
-                reader.StreamClosed += delegate { session.ReleaseToPool(forceDelete: false); };
+                var reader = request.ExecuteReadOrQueryStreamReader(managedTransaction, callSettings);
+                reader.StreamClosed += delegate { _ = Task.Run(() => managedTransaction.DisposeAsync()); };


ManagedTransaction.DisposeAsync() returns a ValueTask. To correctly run this in a background task using Task.Run, you should convert it to a Task using .AsTask(). This ensures the ValueTask is properly handled and avoids potential issues with how Task.Run handles delegates returning ValueTask.

reader.StreamClosed += delegate { _ = Task.Run(() => managedTransaction.DisposeAsync().AsTask()); };

gemini-code-assist · 2026-03-25T21:55:55Z

                case DisposeBehavior.CloseResources:
-                    _session.ReleaseToPool(forceDelete: true);
+                    _ = Task.Run(_transaction.DisposeAsync);
                    break;
                case DisposeBehavior.Default:
-                    // This is a no-op for a detached session.
-                    // We don't have to make a distinction here.
-                    _session.ReleaseToPool(forceDelete: false);
+                    if (!IsShared)
+                    {
+                        _ = Task.Run(_transaction.DisposeAsync);
+                    }
                    break;


ManagedTransaction.DisposeAsync() returns a ValueTask. To correctly run this in a background task using Task.Run, you should convert it to a Task using .AsTask(). Also, Task.Run expects a delegate, so you should use a lambda expression () => .... This ensures the ValueTask is properly awaited in the background thread.

case DisposeBehavior.CloseResources: _ = Task.Run(() => _transaction.DisposeAsync().AsTask()); break; case DisposeBehavior.Default: if (!IsShared) { _ = Task.Run(() => _transaction.DisposeAsync().AsTask()); } break;

gemini-code-assist · 2026-03-25T21:55:55Z

+            return null;
+        }
+
+


This line contains only whitespace and can be removed for code cleanliness.

gemini-code-assist · 2026-03-25T21:55:55Z

 using Google.Protobuf;
 using Google.Protobuf.WellKnownTypes;
 using System;
+using System.CodeDom;


The using System.CodeDom; statement is not used within this file and should be removed to maintain code cleanliness.

robertvoinescu-work · 2026-04-07T17:16:06Z

-    /// </summary>
-    public SpannerTransactionCreationOptions WithIsDetached(bool isDetached) =>
+/// <summary>
+/// Returns a new instance identical to this one except for the value of <see cref="IsDetached"/>.


NIT: Indentation is missing here.

robertvoinescu-work · 2026-04-07T18:22:25Z

                    try
                    {
-                        _sessionPool = await Builder.AcquireSessionPoolAsync().ConfigureAwait(false);
+                            _managedSession = await Builder.MaybeAcquireSessionAsync(requireSession).ConfigureAwait(false);


NIT: White space.

robertvoinescu-work · 2026-04-07T19:29:01Z

 namespace Google.Cloud.Spanner.Data
 {
    /// <summary>
    /// EphemeralTransaction acquires and releases PooledSessions from the provided SessionPool on an as-needed basis.


NIT: We should update this xml doc at some point not to reference SessionPools.

robertvoinescu-work · 2026-04-07T22:30:29Z

+        _spannerSettings = GaxPreconditions.CheckNotNull(spannerSettings, nameof(spannerSettings));
+        _spannerSettings.VersionHeaderBuilder.AppendAssemblyVersion("gccl", typeof(SessionManager));
+
+        _clientFactory = clientFactory ?? ((options, settings) => options.CreateSpannerClientAsync(settings));


Not an issue with your changes, but I think CreatespannerClientAsync should really be taking a CancelationToken and passing it forward to the BuildAsync(CancellationToken) method within. Then all the async methods here could take a cancellation token and we won't have to ignore the cancellation token in the public SpannerConnection.OpenAsync(CancellationToken).

Since we already don't honor that cancellation token and all this is not part of the public surface, I'm fine with not fixing it here & now, just wanted to point it out.

robertvoinescu-work · 2026-04-07T22:59:44Z

        /// <param name="transactionEnlister">Enlistment delegate; may be null.</param>
-        /// <param name="cancellationToken">Cancellation token; may be None</param>
-        private Task OpenAsyncImpl(Action transactionEnlister, CancellationToken cancellationToken)
+        /// <param name="requireSession"></param>


NIT: doc xml is empty for require session.

robertvoinescu-work · 2026-04-07T23:28:06Z

+    /// <summary>
+    /// The Spanner client used by this managed session.
+    /// </summary>
+    public SpannerClient Client => _options.Client;


As another option we could make this internal and then add another public constructor to the ManagedTransaction that takes a ManagedSession Then we won't need to expose the internals of Managed session. But we would then need to add another public constructor so I can see an argument both ways. Your call.

robertvoinescu-work · 2026-04-07T23:36:37Z

+        /// <param name="callSettings">If not null, applies overrides to this RPC call.</param>
+        /// <returns>A <see cref="ReliableStreamReader"/> for this request.</returns>
+        public ReliableStreamReader ExecuteReadOrQueryStreamReader(ManagedTransaction transaction, CallSettings callSettings) =>
+            transaction.ExecuteReadOrQueryStreamReader(this, callSettings);


We should add a null check in ExecuteReadOrQueryStreamReader and in PartitionReadOrQueryAsync:

GaxPreconditions.CheckNotNull(transaction, nameof(transaction)

robertvoinescu-work · 2026-04-07T23:46:21Z

+                return await SessionManager.AcquireSessionAsync(sessionOptions).ConfigureAwait(false);
+            }
+            // If we can't acquire a session, let's make sure we acquire (and cache) the SpannerClient.
+            // Acquiring a session would have used the right cached SpannerClinet or would have created a new on if there was none cached.


NIT: SpannerClinet -> SpannerClient

robertvoinescu-work · 2026-04-08T00:53:01Z

                    try
                    {
-                        _sessionPool = await Builder.AcquireSessionPoolAsync().ConfigureAwait(false);
+                            _managedSession = await Builder.MaybeAcquireSessionAsync(requireSession).ConfigureAwait(false);


NIT: Extra space.

robertvoinescu-work · 2026-04-08T17:31:48Z


        /// <inheritdoc />
-        public override Task OpenAsync(CancellationToken cancellationToken) => OpenAsyncImpl(GetTransactionEnlister(), cancellationToken);
+        public override Task OpenAsync(CancellationToken cancellationToken) => OpenAsyncImpl(GetTransactionEnlister(), requireSession: false);


Could you provide a bit more context on this? Is there a specific reason we are deferring the session creation? Assuming the database is already specified before a connection is opened and future operations will require _managedSession anyway couldn't we just create it eagerly? Doing so would let us drop the boolean flag and simplify the code.

amanda-tarafa added 4 commits March 25, 2026 13:25

feat(Spanner): Support types for Spanner.Data to depend directly on MUX

5b480e3

Note that Spanner.Data already used MUX only, but it did so through the session pool API interface.

refactor(Spanner): Implement SessionPoolManager via SessionManager

da7f2e1

refactor(Spanner): Spanner.Data depends on MUX types directly

e7e527b

refactor(Spanner): Simplify access to a transaction's timestamp bound…

970dd8e

… information

amanda-tarafa requested a review from robertvoinescu-work March 25, 2026 21:52

amanda-tarafa requested a review from a team as a code owner March 25, 2026 21:52

product-auto-label bot added the api: spanner Issues related to the Spanner API. label Mar 25, 2026

gemini-code-assist bot reviewed Mar 25, 2026

View reviewed changes

robertvoinescu-work reviewed Apr 8, 2026

View reviewed changes

Conversation

amanda-tarafa commented Mar 25, 2026

Uh oh!

gemini-code-assist bot commented Mar 25, 2026

Summary of Changes

Highlights

Footnotes

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

gemini-code-assist bot Mar 25, 2026

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Mar 25, 2026

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Mar 25, 2026

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Mar 25, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

robertvoinescu-work Apr 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

robertvoinescu-work Apr 8, 2026 •

edited

Loading