feat: Spanner non-awaiting DDL #15280
Conversation
Adds a method to execute DDL on Spanner without having the client awaiting the long-running operation to finish. This makes it possible to for example start the creation of a secondary index using the client library, without having the library waiting for that operation to finish. Creating a secondary index on a table with a large amount of data can take a very long time (several days).
Summary of ChangesHello @olavloite, I'm Gemini Code Assist1! 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 introduces a significant enhancement to the Spanner client library by providing the ability to execute Data Definition Language (DDL) statements without blocking the client until the operation completes. This is particularly useful for long-running DDL tasks, such as creating secondary indexes on large tables, which can take a considerable amount of time. Users can now initiate these operations and receive a Highlights
Using Gemini Code AssistThe 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
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 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. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![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 a valuable feature for executing Spanner DDL statements without awaiting the completion of the long-running operation. The implementation correctly adds new ExecuteDdl and ExecuteDdlAsync methods and refactors the internal DDL execution logic to support this. My review identifies one issue where the return value of ExecuteNonQueryAsync for DDL statements was changed to an incorrect value; I've provided a suggestion to align it with the base DbCommand contract.
apis/Google.Cloud.Spanner.Data/Google.Cloud.Spanner.Data/SpannerCommand.ExecutableCommand.cs
Outdated
Show resolved
Hide resolved
olavloite
force-pushed
the
spanner-non-blocking-ddl
branch
from
1425944 to
c299d51
Compare
|
Pull request diff results Diff level: Minor Comparing with previous NuGet package Diff level: Minor Finished comparisons for Google.Cloud.Spanner.Data |
1 similar comment
|
Pull request diff results Diff level: Minor Comparing with previous NuGet package Diff level: Minor Finished comparisons for Google.Cloud.Spanner.Data |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a valuable feature for executing Spanner DDL statements without awaiting the completion of the long-running operation. The implementation is clean, follows existing patterns in the codebase, and is accompanied by corresponding integration tests. My review includes a couple of suggestions to enhance the clarity of the documentation for the newly introduced methods.
apis/Google.Cloud.Spanner.Data/Google.Cloud.Spanner.Data/SpannerCommand.cs
Outdated
Show resolved
Hide resolved
apis/Google.Cloud.Spanner.Data/Google.Cloud.Spanner.Data/SpannerCommand.cs
Outdated
Show resolved
Hide resolved
|
Pull request diff results Diff level: Minor Comparing with previous NuGet package Diff level: Minor Finished comparisons for Google.Cloud.Spanner.Data |
amanda-tarafa
left a comment
amanda-tarafa
left a comment
There was a problem hiding this comment.
I've left some specific comments, and there are a lot of files here that seem to come from you dev environment that need to be reverted.
But let's chat more internally.
apis/Google.Cloud.Spanner.Data/Google.Cloud.Spanner.Data/SpannerCommand.ExecutableCommand.cs
Show resolved
Hide resolved
| var response = await databaseAdminClient.CreateDatabaseAsync(request).ConfigureAwait(false); | ||
| response = await response.PollUntilCompletedAsync().ConfigureAwait(false); | ||
| operation = response.RpcMessage; | ||
| if (pollUntilCompleted) |
There was a problem hiding this comment.
Instead of adding this if in two places, let's have this method return the un-polled operation and the wrapping ones poll until completion if they need to.
There was a problem hiding this comment.
Is that really possible in this case, considering the fact that the two methods return two different operation types:
Operation<Database, CreateDatabaseMetadata>Operation<Empty, UpdateDatabaseDdlMetadata>
(Type non-generic Operation class cannot be used for polling)
There was a problem hiding this comment.
If the goal is to remove duplication might I instead suggest a a generic local function to centralize the polling and error-handling logic (at the bottom of this method):
async Task<Operation> HandleLro<TResp, TMeta>(Operation<TResp, TMeta> operation)
{
if (pollUntilCompleted)
{
operation = await operation.PollUntilCompletedAsync().ConfigureAwait(false);
}
if (operation.IsFaulted)
{
throw SpannerException.FromOperationFailedException(operation.Exception);
}
return operation;
}If we go the other way and use the returned Operation to poll later, the calling method will add extra overhead because it needs to establish a new channel since this method shuts down the one it creates in the finally block.
apis/Google.Cloud.Spanner.Data/Google.Cloud.Spanner.Data/SpannerCommand.cs
Outdated
Show resolved
Hide resolved
| /// A reference to the long-running operation that was started for the DDL statement(s). | ||
| /// Note: The operation is null for DropDatabase commands. | ||
| /// </returns> | ||
| public Task<Operation> ExecuteDdlAsync(bool pollUntilCompleted = false, CancellationToken cancellationToken = default) => |
There was a problem hiding this comment.
We usually don't expose the underlying types in the ADO.NET layer. Is it really needed to return an operation? And in that case what part of the Operation is needed?
There was a problem hiding this comment.
Replaced with Task<string> StartDdlAsync
|
Pull request diff results Diff level: Minor Comparing with previous NuGet package Diff level: Minor Finished comparisons for Google.Cloud.Spanner.Data |
olavloite
force-pushed
the
spanner-non-blocking-ddl
branch
from
e2e1643 to
3c984c9
Compare
|
Pull request diff results Diff level: Minor Comparing with previous NuGet package Diff level: Minor Finished comparisons for Google.Cloud.Spanner.Data |
| var response = await databaseAdminClient.CreateDatabaseAsync(request).ConfigureAwait(false); | ||
| response = await response.PollUntilCompletedAsync().ConfigureAwait(false); | ||
| operation = response.RpcMessage; | ||
| if (pollUntilCompleted) |
There was a problem hiding this comment.
If the goal is to remove duplication might I instead suggest a a generic local function to centralize the polling and error-handling logic (at the bottom of this method):
async Task<Operation> HandleLro<TResp, TMeta>(Operation<TResp, TMeta> operation)
{
if (pollUntilCompleted)
{
operation = await operation.PollUntilCompletedAsync().ConfigureAwait(false);
}
if (operation.IsFaulted)
{
throw SpannerException.FromOperationFailedException(operation.Exception);
}
return operation;
}If we go the other way and use the returned Operation to poll later, the calling method will add extra overhead because it needs to establish a new channel since this method shuts down the one it creates in the finally block.
| /// The name of the long-running operation that was started for the DDL statement(s). | ||
| /// Note: The ID is empty for DropDatabase commands. | ||
| /// </returns> | ||
| public Task<string> StartDdlAsync(CancellationToken cancellationToken = default) => |
There was a problem hiding this comment.
NIT: Can we add a param xml element, something like.
/// <param name="cancellationToken">An optional token for canceling the call.</param>There was a problem hiding this comment.
Can we add a test to ensure an operation name is returned also, maybe something that extends this like so:
[Fact]
public async Task StartDdlAsync_ReturnsOperationAndCompletes()
{
string dbName = GenerateDatabaseName();
var operationsClient = DatabaseAdminClient.Create().CreateDatabaseOperationsClient;
using var connection = new SpannerConnection(_fixture.Database.NoDbConnectionString);
try
{
var createCmd = connection.CreateDdlCommand($"CREATE DATABASE {dbName}");
var operationName = await createCmd.StartDdlAsync();
Assert.False(string.IsNullOrEmpty(operationName));
// Wait for the database creation LRO to finish
var rawOperation = await operationsClient.GetOperationAsync(operationName);
var operation = new Operation<Database, CreateDatabaseMetadata>(rawOperation, operationsClient);
var completedOperation = await operation.PollUntilCompletedAsync();
Assert.True(completedOperation.IsCompleted);
Assert.Null(completedOperation.Exception);
}
finally
{
// Always cleanup the created database
var dropCommand = connection.CreateDdlCommand($"DROP DATABASE {dbName}");
await dropCommand.ExecuteNonQueryAsync();
}
// No sessions created, so no session pool.
}3 participants
Adds a method to execute DDL on Spanner without having the client awaiting the long-running operation to finish. This makes it possible to for example start the creation of a secondary index using the client library, without having the library waiting for that operation to finish. Creating a secondary index on a table with a large amount of data can take a very long time (several days).