feat(Spanner): Add support for Cloud Queues by efevans · Pull Request #15850 · googleapis/google-cloud-dotnet

efevans · 2026-08-21T20:17:48Z

b/422231498

Suggested path for reviewing

Take a look at the doc describing public surface changes, link to it in the issue. You can skip the Background section and focus on the others.
Peek at the public Send and Ack methods in SpannerConnection.cs, and then look at the first test of QueueTests.cs` in the integration test project to see a basic usage.
Look at the changes big block change in SpannerCommand.ExecutableCommand around lines 400. This is the portion of code that packages the Send and Ack mutations into the protobuf message wire type
The remainder of the code is simple enough and is largely wiring up the CreateSendCommand public method down to the part that prepares the mutation protobuf types from above

DMN until squash

gemini-code-assist

Code Review

This pull request introduces support for Spanner queues by adding Send and Ack command types, updating the command builder and connection classes, and introducing queue-related test fixtures and integration tests. Feedback on the changes highlights syntax errors in the queue creation SQL statement, an integration test that defines commands but never executes or disposes of them, and a public field that should be private and readonly. Additionally, there is a copy-paste error in the XML documentation for CreateSendCommand, and a potential NullReferenceException in a SpannerCommand constructor because the null check on key is executed after the constructor initializer.

efevans · 2026-08-26T22:19:48Z

/gemini review

gemini-code-assist

Code Review

This pull request introduces support for Spanner Queues by adding Send and Ack commands, updating SpannerCommand and SpannerConnection to handle queue operations, and adding integration tests. The review feedback highlights several issues to address, including a syntax error from a trailing comma in the CREATE QUEUE DDL, a code smell from calling an abstract method in a constructor, timezone-related issues from using DateTime.Now instead of DateTime.UtcNow, hardcoded personal test configurations, a potential NullReferenceException when validating Payload, and various copy-paste errors in exception messages and XML documentation.

efevans · 2026-08-27T17:10:02Z

/gemini review

gemini-code-assist

Code Review

This pull request introduces support for Spanner queues by adding 'Send' and 'Ack' command types to SpannerCommand, along with corresponding integration tests and fixtures. The review feedback highlights several critical issues and improvement opportunities: a syntax error (trailing comma) in the queue creation DDL, synchronous blocking in the test fixture setup, a lack of validation for single-key constraints on Send and Ack commands, a potential runtime failure when validating inferred payload types, and missing property copies in the SpannerCommand copy constructor which would break cloning.

efevans · 2026-08-27T19:28:32Z

/gemini review

gemini-code-assist

Code Review

This pull request introduces support for Spanner database queues by adding Send and Ack command types, along with corresponding connection helper methods, test fixtures, and unit/integration tests. However, there are critical issues in the implementation: the state checks validating that exactly one key is provided for Send and Ack mutations are inverted, which will cause runtime failures when valid inputs are provided. Additionally, a trailing comma in the CREATE QUEUE DDL statement in MutationsQueueFixture.cs will result in a syntax error at runtime.

efevans · 2026-09-17T22:34:50Z

@amanda-tarafa removing you to try cutting down noise until this is ready for review again

amanda-tarafa

Only partially reviewed, but better to get the changes I request in before continuing. These are bound to change things further own.

amanda-tarafa · 2026-09-25T17:46:15Z

+        /// </summary>
+        /// <param name="databaseQueue">The name of the queue to send a message to. Must not be null.</param>
+        /// <param name="parameters">A collection of <see cref="SpannerParameter"/> that comprise
+        /// the message key and Payload. Requires a Payload and at least one non-payload parameter.</param>


Do we need to validate that the key is present? Even if it's required by Spanner, if there's nothing and we set it to nothing, then Spanner will fail.

And actually, we can do that for Payload as well, we attempt to extract payload, but if it's not there, we just set it to nothin and let Spanner fail.

Yes, I think this approach matches better our philosophy of keeping validations to a minimum client side. And when you make these changes, make sure to update this comment here to say something like "A collection of parameters that should contain the key elements and the payload."

And same for the other.

amanda-tarafa · 2026-09-25T17:53:20Z

+        internal static SpannerCommand ForSendCommand(
+            SpannerCommandTextBuilder commandTextBuilder,
+            SpannerConnection connection,
+            SpannerParameterCollection parameters,
+            SpannerTransaction transaction = null) => new(commandTextBuilder, connection, transaction, parameters);
+
+        internal static SpannerCommand ForAckCommand(
+            SpannerCommandTextBuilder commandTextBuilder,
+            SpannerConnection connection,
+            SpannerParameterCollection parameters,
+            SpannerTransaction transaction = null) => new(commandTextBuilder, connection, transaction, parameters);
+


Why do we add this layer here that we don't have for any of the other commands. Let's just use the SpannerCommand constructor anywhere when we create send and ack.

amanda-tarafa · 2026-09-25T17:59:23Z

+        /// Note: "Insert" and "Delete" are never treated as Send and Ack even when the
+        /// target table is internally a queue.


Suggested change

/// Note: "Insert" and "Delete" are never treated as Send and Ack even when the

/// target table is internally a queue.

/// Note: `insert <name>` and `delete <name>` are always interpreted as insert and delete mutations, never as send or ack mutations.

amanda-tarafa · 2026-09-25T18:08:26Z

+        /// <summary>
+        /// Optional configurations for Send mutations.
+        /// </summary>
+        public SendOptions SendOptions { get; set; }
+
+        /// <summary>
+        /// Optional configurations for Ack mutations.
+        /// </summary>
+        public AckOptions AckOptions { get; set; }
+


In docs, add a note, like the one we have in DirectedRead for instance, saying that these will be ignored for all other operations.

amanda-tarafa · 2026-09-25T18:16:29Z

+        /// Note: Insert and Delete are never treated as Send and Ack even when the
+        /// target table is internally a queue.


Same wording as in the other one.

amanda-tarafa · 2026-09-25T18:25:22Z

+        private const string SendCommand = "SEND";
+        private const string AckCommand = "ACK";


We are not parsing these, so why do we need these constants for? We cannot represent these as text, we should use the empty string for the text command here, similar to what we do for Read IIRC.

amanda-tarafa · 2026-09-25T18:33:38Z

+        /// <param name="queue">The name of the Spanner database queue for which messages will be acked. Must not be null.</param>
+        /// <returns>A <see cref="SpannerCommandTextBuilder"/> representing a <see cref="F:SpannerCommandType.Ack"/> Spanner command.</returns>
+        public static SpannerCommandTextBuilder CreateAckTextBuilder(string queue) =>
+            CreateBuilderForTableDml(AckCommand, SpannerCommandType.Ack, queue);


I think you need to create a new method CreateBuilderForQueueMutation that only receives the type and the name of the queue. We need to add a new public property here called TargetQueue that's equivalent to TargetTable but only one of those can be set at any given time.

product-auto-label Bot added the api: spanner Issues related to the Spanner API. label Aug 21, 2026

gemini-code-assist Bot reviewed Aug 21, 2026

View reviewed changes

efevans force-pushed the spanner-queues branch 16 times, most recently from 6fde46a to d858058 Compare August 26, 2026 21:22

gemini-code-assist Bot reviewed Aug 26, 2026

View reviewed changes

efevans force-pushed the spanner-queues branch from d858058 to 47daa5d Compare August 27, 2026 16:40

googleapis deleted a comment from gemini-code-assist Bot Aug 27, 2026

efevans force-pushed the spanner-queues branch 2 times, most recently from 085d8cc to e83cef5 Compare August 27, 2026 17:01

gemini-code-assist Bot reviewed Aug 27, 2026

View reviewed changes

efevans force-pushed the spanner-queues branch 2 times, most recently from f2a5bc7 to e848450 Compare August 27, 2026 19:25

gemini-code-assist Bot reviewed Aug 27, 2026

View reviewed changes

efevans force-pushed the spanner-queues branch from 69a5e27 to 90dd96d Compare September 17, 2026 21:55

efevans removed the request for review from amanda-tarafa September 17, 2026 22:34

efevans force-pushed the spanner-queues branch from 1d43440 to 51a10db Compare September 21, 2026 20:48

efevans marked this pull request as ready for review September 21, 2026 21:23

efevans added the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Sep 24, 2026

efevans added 16 commits September 24, 2026 13:00

feat(Spanner): Add support for Cloud Queues

a1b87e8

Refactor Payload to a Payload type

21bc551

Properly queue payload test types with fixture types

9fea330

Remove unncessary tests

2401af5

Update to latest doc design

c476449

Fix clone failing on send and ack options being null

9263245

Add note in XML doc about string commands never parsing as Send or Ack

6004b0f

Update tests with unique ids

d1e17f7

Add back in BOM

dd0ba84

Fix updated tests setting delay time relative to overall test start time

3c0440c

Revert changed constructor to no long allow send or ack

2fb6ec7

Fix error message for ack parameter count

fc74771

Fix misspellings

73a1a5f

Remove integration tests for testing ignorenotfound and deliverat

0c27318

Add integration tests for insert and delete DML

05e79c6

Convert conditional in test to MemberData

5fa8509

efevans force-pushed the spanner-queues branch from 51b22de to 5fa8509 Compare September 24, 2026 20:00

efevans added 2 commits September 24, 2026 13:57

Move test datetime out of class scope

bc2329c

Change an assert from assert true to assert equal

91312ec

amanda-tarafa requested changes Sep 25, 2026

View reviewed changes

efevans added 4 commits September 29, 2026 14:02

Remove client-side validation for presence of key and Payload

5427383

Remove unnecessary method

d660ad4

Fix inconsistent xml doc string

926f7bd

Use queues for SpannerCommandTextBuilder

ff11548

		/// Note: "Insert" and "Delete" are never treated as Send and Ack even when the
		/// target table is internally a queue.

	/// Note: "Insert" and "Delete" are never treated as Send and Ack even when the
	/// target table is internally a queue.
	/// Note: `insert <name>` and `delete <name>` are always interpreted as insert and delete mutations, never as send or ack mutations.

		/// Note: Insert and Delete are never treated as Send and Ack even when the
		/// target table is internally a queue.

		private const string SendCommand = "SEND";
		private const string AckCommand = "ACK";

Conversation

efevans commented Aug 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

efevans commented Aug 26, 2026

Uh oh!

gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

efevans commented Aug 27, 2026

Uh oh!

gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

efevans commented Aug 27, 2026

Uh oh!

gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

Uh oh!

Uh oh!

Uh oh!

efevans commented Sep 17, 2026

Uh oh!

amanda-tarafa left a comment

Choose a reason for hiding this comment

Uh oh!

amanda-tarafa Sep 25, 2026

Choose a reason for hiding this comment

Uh oh!

amanda-tarafa Sep 25, 2026

Choose a reason for hiding this comment

Uh oh!

amanda-tarafa Sep 25, 2026

Choose a reason for hiding this comment

Uh oh!

amanda-tarafa Sep 25, 2026

Choose a reason for hiding this comment

Uh oh!

amanda-tarafa Sep 25, 2026

Choose a reason for hiding this comment

Uh oh!

amanda-tarafa Sep 25, 2026

Choose a reason for hiding this comment

Uh oh!

amanda-tarafa Sep 25, 2026

Choose a reason for hiding this comment

Uh oh!

This branch has not been deployed

Reviewers

Assignees

Labels

Projects

Milestone

efevans commented Aug 21, 2026 •

edited

Loading