Skip to content
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.

Sign up for GitHub

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

feat: migrate connection API to use the new auto-generated admin clients. #2879

Closed
wants to merge 24 commits into from
Closed

feat: migrate connection API to use the new auto-generated admin clients. #2879

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
edc5bbf
fix: prevent illegal negative timeout values into thread sleep() meth…
arpan14 Feb 6, 2023
49a85df
Merge pull request #1 from arpan14/retryerror
arpan14 Feb 8, 2023
4cd497b
Fixing lint issues.
arpan14 Feb 8, 2023
4a6aa8e
Merge branch 'googleapis:main' into main
arpan14 Mar 13, 2023
b2aa09d
Merge branch 'googleapis:main' into main
arpan14 Mar 15, 2023
8d6d71e
Merge branch 'googleapis:main' into main
arpan14 May 9, 2023
77e6e7d
Merge branch 'googleapis:main' into main
arpan14 Jul 17, 2023
e8b7fad
Merge branch 'googleapis:main' into main
arpan14 Jul 25, 2023
8aa84e1
Merge branch 'googleapis:main' into main
arpan14 Oct 10, 2023
57fd405
Merge branch 'googleapis:main' into main
arpan14 Oct 27, 2023
1253563
Merge branch 'googleapis:main' into main
arpan14 Nov 20, 2023
d4f6a60
Merge branch 'googleapis:main' into main
arpan14 Dec 15, 2023
3efaf7c
Merge branch 'googleapis:main' into main
arpan14 Dec 26, 2023
f41b39f
Merge branch 'googleapis:main' into main
arpan14 Jan 3, 2024
7e3287f
Merge branch 'googleapis:main' into main
arpan14 Jan 13, 2024
7edd24d
Merge branch 'googleapis:main' into main
arpan14 Feb 13, 2024
451d2c1
feat: support emulator with autogenerated admin clients.
arpan14 Feb 9, 2024
5ca5d6d
chore: modifying public interfaces and adding an integration test.
arpan14 Feb 10, 2024
5272aa3
chore: add deprecated annotations and add docs.
arpan14 Feb 10, 2024
14f6080
feat: migrate connection API to use the new auto-generated admin clie…
arpan14 Feb 12, 2024
f1ec78c
chore: rebase fixes.
arpan14 Feb 13, 2024
acb847a
Update Spanner.java
arpan14 Feb 14, 2024
2da9e18
chore: fix clirr errors.
arpan14 Feb 13, 2024
a36ed1c
chore: fix channel leak errors.
arpan14 Feb 19, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions google-cloud-spanner/clirr-ignored-differences.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,11 @@
<className>com/google/cloud/spanner/DatabaseClient</className>
<method>com.google.cloud.spanner.Dialect getDialect()</method>
</difference>
<difference>
<differenceType>7012</differenceType>
<className>com/google/cloud/spanner/DatabaseClient</className>
<method>com.google.spanner.admin.database.v1.DatabaseDialect getDatabaseDialect()</method>
</difference>
<difference>
<differenceType>7012</differenceType>
<className>com/google/cloud/spanner/BatchReadOnlyTransaction</className>
Expand Down
10 changes: 10 additions & 0 deletions google-cloud-spanner/src/main/java/com/google/cloud/spanner/DatabaseClient.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
import com.google.cloud.spanner.Options.RpcPriority;
import com.google.cloud.spanner.Options.TransactionOption;
import com.google.cloud.spanner.Options.UpdateOption;
import com.google.spanner.admin.database.v1.DatabaseDialect;
import com.google.spanner.v1.BatchWriteResponse;

/**
Expand All @@ -38,6 +39,15 @@ default Dialect getDialect() {
throw new UnsupportedOperationException("method should be overwritten");
}

/**
* Returns the SQL dialect that is used by the database.
*
* @return the SQL dialect that is used by the database.
*/
default DatabaseDialect getDatabaseDialect() {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we really need this? I think it will be confusing for customers when to use which of these two methods (getDialect() and getDatabaseDialect()). Could we instead instruct users to use Dialect.toProto() instead when they need the proto version?

throw new UnsupportedOperationException("method should be overwritten");
}

/**
* Returns the {@link DatabaseRole} used by the client connection. The database role that is used
* determines the access permissions that a connection has. This can for example be used to create
Expand Down
6 changes: 6 additions & 0 deletions google-cloud-spanner/src/main/java/com/google/cloud/spanner/DatabaseClientImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
import com.google.common.annotations.VisibleForTesting;
import com.google.common.base.Function;
import com.google.common.util.concurrent.ListenableFuture;
import com.google.spanner.admin.database.v1.DatabaseDialect;
import com.google.spanner.v1.BatchWriteResponse;
import javax.annotation.Nullable;

Expand Down Expand Up @@ -57,6 +58,11 @@ public Dialect getDialect() {
return pool.getDialect();
}

@Override
public DatabaseDialect getDatabaseDialect() {
return getDialect().toProto();
}

@Override
@Nullable
public String getDatabaseRole() {
Expand Down
1 change: 1 addition & 0 deletions google-cloud-spanner/src/main/java/com/google/cloud/spanner/Spanner.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,7 @@ public interface Spanner extends Service<SpannerOptions>, AutoCloseable {
*
* @return {@code InstanceAdminClient}
*/

/*
* <!--SNIPPET get_instance_admin_client-->
* <pre>{@code
Expand Down
19 changes: 16 additions & 3 deletions google-cloud-spanner/src/main/java/com/google/cloud/spanner/connection/ConnectionImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@
import com.google.cloud.spanner.Statement;
import com.google.cloud.spanner.TimestampBound;
import com.google.cloud.spanner.TimestampBound.Mode;
import com.google.cloud.spanner.admin.database.v1.DatabaseAdminClient;
import com.google.cloud.spanner.connection.AbstractStatementParser.ParsedStatement;
import com.google.cloud.spanner.connection.AbstractStatementParser.StatementType;
import com.google.cloud.spanner.connection.StatementExecutor.StatementTimeout;
Expand Down Expand Up @@ -186,6 +187,7 @@ static UnitOfWorkType of(TransactionMode transactionMode) {

private final Spanner spanner;
private final DdlClient ddlClient;
private final DatabaseAdminClient databaseAdminClient;
private final DatabaseClient dbClient;
private final BatchClient batchClient;
private boolean autocommit;
Expand Down Expand Up @@ -273,7 +275,8 @@ static UnitOfWorkType of(TransactionMode transactionMode) {
this.autoPartitionMode = options.isAutoPartitionMode();
this.maxPartitions = options.getMaxPartitions();
this.maxPartitionedParallelism = options.getMaxPartitionedParallelism();
this.ddlClient = createDdlClient();
this.databaseAdminClient = spanner.createDatabaseAdminClient();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will create a separate admin client for each connection that is created, including underlying gRPC channels. That means that an application that uses a JDBC connection that opens 100 connections, all of a sudden uses 4 channels (for the normal Spanner instance) + 100 (or 400? I don't remember exactly what channel settings we are using for the generated admin clients) channels for the 100 DatabaseAdmin clients.

It also means that creating a new Connection is suddenly a much heavier operation, meaning that applications that do not use connection pooling could potentially experience a performance degradation.

Currently, creating a Connection instance is an non-blocking method, and creating multiple connections to the same database does not incur much cost (it only costs the bit of memory that is required to keep track of the Connection state, but it does not create any additional physical connections to Spanner).

this.ddlClient = createDdlClient(databaseAdminClient);
setDefaultTransactionOptions();
}

Expand All @@ -283,6 +286,7 @@ static UnitOfWorkType of(TransactionMode transactionMode) {
ConnectionOptions options,
SpannerPool spannerPool,
DdlClient ddlClient,
DatabaseAdminClient databaseAdminClient,
DatabaseClient dbClient,
BatchClient batchClient) {
this.leakedException =
Expand All @@ -291,6 +295,7 @@ static UnitOfWorkType of(TransactionMode transactionMode) {
new StatementExecutor(options.isUseVirtualThreads(), Collections.emptyList());
this.spannerPool = Preconditions.checkNotNull(spannerPool);
this.options = Preconditions.checkNotNull(options);
this.databaseAdminClient = Preconditions.checkNotNull(databaseAdminClient);
this.spanner = spannerPool.getSpanner(options, this);
this.ddlClient = Preconditions.checkNotNull(ddlClient);
this.dbClient = Preconditions.checkNotNull(dbClient);
Expand All @@ -306,9 +311,10 @@ Spanner getSpanner() {
return this.spanner;
}

private DdlClient createDdlClient() {
private DdlClient createDdlClient(DatabaseAdminClient databaseAdminClient) {
return DdlClient.newBuilder()
.setDatabaseAdminClient(spanner.getDatabaseAdminClient())
.setDatabaseAdminClient(databaseAdminClient)
.setProjectId(options.getProjectId())
.setInstanceId(options.getInstanceId())
.setDatabaseName(options.getDatabaseName())
.build();
Expand Down Expand Up @@ -362,6 +368,13 @@ public ApiFuture<Void> closeAsync() {
statementExecutor.shutdown();
leakedException = null;
spannerPool.removeConnection(options, this);
try {
if (!databaseAdminClient.isTerminated() || !databaseAdminClient.isShutdown()) {
databaseAdminClient.close();
}
} catch (RuntimeException ex) {
throw SpannerExceptionFactory.newSpannerException(ex);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are we sure that we want to propagate this exception? Or should we just ignore it? Are there any potential things that the user might have done wrong that could cause an exception here? If yes, then it is probably best to propagate it. If this is more of a 'better-safe-than-sorry' and/or 'this sometimes randomly causes an exception, no idea why'-kind of situation, then I would suggest that we ignore the exception.

But see also above: I don't think we should create a separate DatabaseAdminClient for each connection that is created.

}
return ApiFutures.transform(
ApiFutures.allAsList(futures), ignored -> null, MoreExecutors.directExecutor());
}
Expand Down
5 changes: 3 additions & 2 deletions google-cloud-spanner/src/main/java/com/google/cloud/spanner/connection/DdlBatch.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@
import com.google.cloud.spanner.connection.AbstractStatementParser.StatementType;
import com.google.common.annotations.VisibleForTesting;
import com.google.common.base.Preconditions;
import com.google.protobuf.Empty;
import com.google.spanner.admin.database.v1.DatabaseAdminGrpc;
import com.google.spanner.admin.database.v1.UpdateDatabaseDdlMetadata;
import java.util.ArrayList;
Expand Down Expand Up @@ -212,7 +213,7 @@ public ApiFuture<long[]> runBatchAsync(CallType callType) {
Callable<long[]> callable =
() -> {
try {
OperationFuture<Void, UpdateDatabaseDdlMetadata> operation =
OperationFuture<Empty, UpdateDatabaseDdlMetadata> operation =
ddlClient.executeDdl(statements);
try {
// Wait until the operation has finished.
Expand All @@ -236,7 +237,7 @@ public ApiFuture<long[]> runBatchAsync(CallType callType) {
callType, RUN_BATCH_STATEMENT, callable, DatabaseAdminGrpc.getUpdateDatabaseDdlMethod());
}

long[] extractUpdateCounts(OperationFuture<Void, UpdateDatabaseDdlMetadata> operation) {
long[] extractUpdateCounts(OperationFuture<Empty, UpdateDatabaseDdlMetadata> operation) {
try {
return extractUpdateCounts(operation.getMetadata().get());
} catch (Throwable t) {
Expand Down
39 changes: 30 additions & 9 deletions google-cloud-spanner/src/main/java/com/google/cloud/spanner/connection/DdlClient.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,14 +17,18 @@
package com.google.cloud.spanner.connection;

import com.google.api.gax.longrunning.OperationFuture;
import com.google.cloud.spanner.Database;
import com.google.cloud.spanner.DatabaseAdminClient;
import com.google.cloud.spanner.Dialect;
import com.google.cloud.spanner.ErrorCode;
import com.google.cloud.spanner.SpannerExceptionFactory;
import com.google.cloud.spanner.admin.database.v1.DatabaseAdminClient;
import com.google.common.base.Preconditions;
import com.google.common.base.Strings;
import com.google.protobuf.Empty;
import com.google.spanner.admin.database.v1.CreateDatabaseMetadata;
import com.google.spanner.admin.database.v1.CreateDatabaseRequest;
import com.google.spanner.admin.database.v1.Database;
import com.google.spanner.admin.database.v1.DatabaseDialect;
import com.google.spanner.admin.database.v1.DatabaseName;
import com.google.spanner.admin.database.v1.InstanceName;
import com.google.spanner.admin.database.v1.UpdateDatabaseDdlMetadata;
import java.util.Collections;
import java.util.List;
Expand All @@ -35,11 +39,13 @@
*/
class DdlClient {
private final DatabaseAdminClient dbAdminClient;
private final String projectId;
private final String instanceId;
private final String databaseName;

static class Builder {
private DatabaseAdminClient dbAdminClient;
private String projectId;
private String instanceId;
private String databaseName;

Expand All @@ -51,6 +57,13 @@ Builder setDatabaseAdminClient(DatabaseAdminClient client) {
return this;
}

Builder setProjectId(String projectId) {
Preconditions.checkArgument(
!Strings.isNullOrEmpty(projectId), "Empty projectId is not allowed");
this.projectId = projectId;
return this;
}

Builder setInstanceId(String instanceId) {
Preconditions.checkArgument(
!Strings.isNullOrEmpty(instanceId), "Empty instanceId is not allowed");
Expand All @@ -67,6 +80,7 @@ Builder setDatabaseName(String name) {

DdlClient build() {
Preconditions.checkState(dbAdminClient != null, "No DatabaseAdminClient specified");
Preconditions.checkState(!Strings.isNullOrEmpty(projectId), "No ProjectId specified");
Preconditions.checkState(!Strings.isNullOrEmpty(instanceId), "No InstanceId specified");
Preconditions.checkArgument(
!Strings.isNullOrEmpty(databaseName), "No database name specified");
Expand All @@ -80,29 +94,36 @@ static Builder newBuilder() {

private DdlClient(Builder builder) {
this.dbAdminClient = builder.dbAdminClient;
this.projectId = builder.projectId;
this.instanceId = builder.instanceId;
this.databaseName = builder.databaseName;
}

OperationFuture<Database, CreateDatabaseMetadata> executeCreateDatabase(
String createStatement, Dialect dialect) {
String createStatement, DatabaseDialect databaseDialect) {
Preconditions.checkArgument(isCreateDatabaseStatement(createStatement));
return dbAdminClient.createDatabase(
instanceId, createStatement, dialect, Collections.emptyList());
CreateDatabaseRequest createDatabaseRequest =
CreateDatabaseRequest.newBuilder()
.setParent(InstanceName.of(projectId, instanceId).toString())
.setCreateStatement(createStatement)
.setDatabaseDialect(databaseDialect)
.build();
return dbAdminClient.createDatabaseAsync(createDatabaseRequest);
}

/** Execute a single DDL statement. */
OperationFuture<Void, UpdateDatabaseDdlMetadata> executeDdl(String ddl) {
OperationFuture<Empty, UpdateDatabaseDdlMetadata> executeDdl(String ddl) {
return executeDdl(Collections.singletonList(ddl));
}

/** Execute a list of DDL statements as one operation. */
OperationFuture<Void, UpdateDatabaseDdlMetadata> executeDdl(List<String> statements) {
OperationFuture<Empty, UpdateDatabaseDdlMetadata> executeDdl(List<String> statements) {
if (statements.stream().anyMatch(DdlClient::isCreateDatabaseStatement)) {
throw SpannerExceptionFactory.newSpannerException(
ErrorCode.INVALID_ARGUMENT, "CREATE DATABASE is not supported in a DDL batch");
}
return dbAdminClient.updateDatabaseDdl(instanceId, databaseName, statements, null);
return dbAdminClient.updateDatabaseDdlAsync(
DatabaseName.of(projectId, instanceId, databaseName), statements);
}

/** Returns true if the statement is a `CREATE DATABASE ...` statement. */
Expand Down
2 changes: 1 addition & 1 deletion ...cloud-spanner/src/main/java/com/google/cloud/spanner/connection/SingleUseTransaction.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -372,7 +372,7 @@ public ApiFuture<Void> executeDdlAsync(CallType callType, final ParsedStatement
if (DdlClient.isCreateDatabaseStatement(ddl.getSqlWithoutComments())) {
operation =
ddlClient.executeCreateDatabase(
ddl.getSqlWithoutComments(), dbClient.getDialect());
ddl.getSqlWithoutComments(), dbClient.getDatabaseDialect());
} else {
operation = ddlClient.executeDdl(ddl.getSqlWithoutComments());
}
Expand Down
5 changes: 4 additions & 1 deletion ...loud-spanner/src/test/java/com/google/cloud/spanner/connection/AutocommitDmlModeTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@
import com.google.cloud.spanner.TransactionManager;
import com.google.cloud.spanner.TransactionRunner;
import com.google.cloud.spanner.TransactionRunner.TransactionCallable;
import com.google.cloud.spanner.admin.database.v1.DatabaseAdminClient;
import org.junit.Test;
import org.junit.runner.RunWith;
import org.junit.runners.JUnit4;
Expand All @@ -57,6 +58,7 @@ private ConnectionImpl createConnection(ConnectionOptions options) {
when(spannerPool.getSpanner(any(ConnectionOptions.class), any(ConnectionImpl.class)))
.thenReturn(spanner);
DdlClient ddlClient = mock(DdlClient.class);
DatabaseAdminClient databaseAdminClient = mock(DatabaseAdminClient.class);
TransactionRunner txRunner = mock(TransactionRunner.class);
when(dbClient.readWriteTransaction()).thenReturn(txRunner);
when(txRunner.run(any(TransactionCallable.class)))
Expand All @@ -71,7 +73,8 @@ private ConnectionImpl createConnection(ConnectionOptions options) {
when(txManager.begin()).thenReturn(txContext);
when(dbClient.transactionManager()).thenReturn(txManager);

return new ConnectionImpl(options, spannerPool, ddlClient, dbClient, mock(BatchClient.class));
return new ConnectionImpl(
options, spannerPool, ddlClient, databaseAdminClient, dbClient, mock(BatchClient.class));
}

@Test
Expand Down
Loading
Loading