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

Jump to bottom

feat: support public methods to use autogenerated admin clients. #2878

Merged
merged 34 commits into from
Feb 13, 2024
Merged

feat: support public methods to use autogenerated admin clients. #2878

Show file tree
Hide file tree
Changes from 28 commits
Commits
Show all changes
34 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
5d090ea
feat: support emulator with autogenerated admin clients.
arpan14 Feb 9, 2024
5c9c2e4
chore: modifying public interfaces and adding an integration test.
arpan14 Feb 10, 2024
bd4156b
chore: add deprecated annotations and add docs.
arpan14 Feb 10, 2024
8b5d800
chore: making interface methods default.
arpan14 Feb 12, 2024
500fd02
chore: add clirr ignore checks for public methods.
arpan14 Feb 12, 2024
832077e
Merge branch 'main' into admin-emulator-support
arpan14 Feb 12, 2024
0be2719
Update google-cloud-spanner/src/main/java/com/google/cloud/spanner/Sp…
arpan14 Feb 12, 2024
ab05f88
Update google-cloud-spanner/src/main/java/com/google/cloud/spanner/Sp…
arpan14 Feb 12, 2024
bb41cf7
Update google-cloud-spanner/src/main/java/com/google/cloud/spanner/Sp…
arpan14 Feb 12, 2024
3e8caa2
Update google-cloud-spanner/src/main/java/com/google/cloud/spanner/Sp…
arpan14 Feb 12, 2024
f71a4c1
chore: address PR comments.
arpan14 Feb 12, 2024
8330718
chore: decouple client stub objects and rely on common stub settings …
arpan14 Feb 12, 2024
d7f70fa
chore: remove unused map objects.
arpan14 Feb 12, 2024
07c9cd1
chore: fix broken unit tests.
arpan14 Feb 12, 2024
447ec3a
chore: handle case when client is terminated/shutdown
arpan14 Feb 12, 2024
3dd7017
chore: change to create methods and avoid stale instances.
arpan14 Feb 13, 2024
543fed9
Revert "chore: fix broken unit tests."
arpan14 Feb 13, 2024
53d4c7b
test: add more unit tests.
arpan14 Feb 13, 2024
d8d6a5b
🦉 Updates from OwlBot post-processor
gcf-owl-bot[bot] Feb 13, 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
27 changes: 25 additions & 2 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 @@ -539,6 +539,30 @@
<className>com/google/cloud/spanner/Dialect</className>
<method>java.lang.String getDefaultSchema()</method>
</difference>
<!-- Exposing auto-generated admin/instance clients for admin operations -->
<difference>
<differenceType>7012</differenceType>
<className>com/google/cloud/spanner/Spanner</className>
<method>com.google.cloud.spanner.admin.database.v1.DatabaseAdminClient databaseAdminClient()</method>
</difference>
<difference>
<differenceType>7012</differenceType>
<className>com/google/cloud/spanner/Spanner</className>
<method>com.google.cloud.spanner.admin.instance.v1.InstanceAdminClient instanceAdminClient()</method>
</difference>

<!-- (Internal change, use stub objects for generating auto-generated client object) -->
<difference>
<differenceType>7012</differenceType>
<className>com/google/cloud/spanner/spi/v1/SpannerRpc</className>
<method>com.google.cloud.spanner.admin.database.v1.stub.DatabaseAdminStubSettings getDatabaseAdminStubSettings()</method>
</difference>
<difference>
<differenceType>7012</differenceType>
<className>com/google/cloud/spanner/spi/v1/SpannerRpc</className>
<method>com.google.cloud.spanner.admin.instance.v1.stub.InstanceAdminStubSettings getInstanceAdminStubSettings()</method>
</difference>

<difference>
<differenceType>7005</differenceType>
<className>com/google/cloud/spanner/PartitionedDmlTransaction</className>
Expand All @@ -556,6 +580,5 @@
<differenceType>7012</differenceType>
<className>com/google/cloud/spanner/connection/Connection</className>
<method>void setDirectedRead(com.google.spanner.v1.DirectedReadOptions)</method>
</difference>

</difference>
</differences>
51 changes: 49 additions & 2 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 @@ -26,7 +26,12 @@
* quota.
*/
public interface Spanner extends Service<SpannerOptions>, AutoCloseable {
/** Returns a {@code DatabaseAdminClient} to do admin operations on Cloud Spanner databases. */

/**
* Returns a {@code DatabaseAdminClient} to execute admin operations on Cloud Spanner databases.
*
* @return {@code DatabaseAdminClient}
*/
/*
* <!--SNIPPET get_dbadmin_client-->
* <pre>{@code
Expand All @@ -38,7 +43,30 @@ public interface Spanner extends Service<SpannerOptions>, AutoCloseable {
*/
DatabaseAdminClient getDatabaseAdminClient();

/** Returns an {@code InstanceAdminClient} to do admin operations on Cloud Spanner instances. */
/**
* Returns a {@link com.google.cloud.spanner.admin.database.v1.DatabaseAdminClient} to execute
* admin operations on Cloud Spanner databases.
*
* @return {@link com.google.cloud.spanner.admin.database.v1.DatabaseAdminClient}
*/
/*
* <!--SNIPPET get_dbadmin_client-->
* <pre>{@code
* SpannerOptions options = SpannerOptions.newBuilder().build();
* Spanner spanner = options.getService();
* DatabaseAdminClient dbAdminClient = spanner.databaseAdminClient();
* }</pre>
* <!--SNIPPET get_dbadmin_client-->
*/
default com.google.cloud.spanner.admin.database.v1.DatabaseAdminClient databaseAdminClient() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This method returns an instance of com.google.cloud.spanner.admin.database.v1.DatabaseAdminClient. That interface implements AutoCloseable. That again means that most users will be inclined to put it in a try-with-resources block, or in some other way close it when they are done. If they do that, then the underlying stub that is used by the client will be closed, which again is a shared object, and means that all existing and also future instances that will be returned by this method will use a closed stub.

So we need to make a choice here whether:

  1. We document that users should never call close() on the instance that is returned.
  2. OR: Create a new stub for each new instance that is returned by this method.
  3. OR: Create/use a small wrapper object around the instance that we are returning here that overrides the default close() behavior. The modified behavior could either be to do nothing, or to throw an exception that indicates that this instance is managed by the Spanner instance that created it.

I think that option 3 is the safest option.

Copy link
Collaborator Author

@arpan14 arpan14 Feb 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Thanks for the catch!

Thinking a bit more on the issue, I think its due to the dependency on the stub making it a shared resource when it should not have been one. Another approach which I explored was to make use of the shared DatabaseAdminStubSettings and InstanceAdminStubSettings objects and creating a new stub objects from these settings by doing something like gapicRpc.getDatabaseAdminStubSettings().createStub() . This will ensure we avoid the coupling and at the same time are able to re-use all the code of GapicSpannerRpc for creating the settings.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Based on offline conversations, we found an additional issue. For ex - refer the below snippet where we would still up see the shared stub behavior.

void doSomeAdminStuff() {
  try (DatabaseAdminClient client = spanner.databaseAdminClient()) {
    client.listDatabases();
    doSomethingElse();
    client.dropDatabase(...);
  }
}

void doSomethingElse() {
  try (DatabaseAdminClient client = spanner.databaseAdminClient()) {
    client.updateDatabase(...);
  }
}

I explored two other approaches based on @olavloite 's suggestion

  1. Creating a ManagedDatabaseAdminClient which extends the DatabaseAdminClient class and overrides the close() method. This is not possible to implement since the method is marked as final and hence we cannot override it.
  2. Using dynamic proxy classes (reference - https://www.baeldung.com/java-dynamic-proxies#invocation-handler-via-lambda-expressions ). This is not possible since dynamic proxies only work for classes that implement some interface and in this case DatabaseAdminClient does not implement any interface that can supply all its methods.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We are settling for an approach where we return a new instance each time a client object is requested. This makes sure we don't run into any edge cases (like discussed above). At the same time, we have captured in the method documentation that customers may choose to cache the instances based on their application side code.

throw new UnsupportedOperationException("Not implemented");
}

/**
* Returns an {@code InstanceAdminClient} to execute admin operations on Cloud Spanner instances.
*
* @return {@code InstanceAdminClient}
*/
/*
* <!--SNIPPET get_instance_admin_client-->
* <pre>{@code
Expand All @@ -50,6 +78,25 @@ public interface Spanner extends Service<SpannerOptions>, AutoCloseable {
*/
InstanceAdminClient getInstanceAdminClient();

/**
* Returns an {@link com.google.cloud.spanner.admin.instance.v1.InstanceAdminClient} to execute
* admin operations on Cloud Spanner instances.
*
* @return {@link com.google.cloud.spanner.admin.instance.v1.InstanceAdminClient}
*/
/*
* <!--SNIPPET get_instance_admin_client-->
* <pre>{@code
* SpannerOptions options = SpannerOptions.newBuilder().build();
* Spanner spanner = options.getService();
* InstanceAdminClient instanceAdminClient = spanner.instanceAdminClient();
* }</pre>
* <!--SNIPPET get_instance_admin_client-->
*/
default com.google.cloud.spanner.admin.instance.v1.InstanceAdminClient instanceAdminClient() {
throw new UnsupportedOperationException("Not implemented");
}

/**
* Returns a {@code DatabaseClient} for the given database. It uses a pool of sessions to talk to
* the database.
Expand Down
36 changes: 36 additions & 0 deletions google-cloud-spanner/src/main/java/com/google/cloud/spanner/SpannerImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@
import io.opencensus.trace.Tracing;
import io.opentelemetry.api.common.Attributes;
import io.opentelemetry.api.common.AttributesBuilder;
import java.io.IOException;
import java.util.ArrayList;
import java.util.HashMap;
import java.util.List;
Expand Down Expand Up @@ -107,6 +108,9 @@ private static String nextDatabaseClientId(DatabaseId databaseId) {
private final DatabaseAdminClient dbAdminClient;
private final InstanceAdminClient instanceClient;

private final com.google.cloud.spanner.admin.database.v1.DatabaseAdminClient databaseAdminClient;
private final com.google.cloud.spanner.admin.instance.v1.InstanceAdminClient instanceAdminClient;

/**
* Exception class used to track the stack trace at the point when a Spanner instance is closed.
* This exception will be thrown if a user tries to use any resources that were returned by this
Expand Down Expand Up @@ -135,6 +139,8 @@ static final class ClosedException extends RuntimeException {
this.dbAdminClient = new DatabaseAdminClientImpl(options.getProjectId(), gapicRpc);
this.instanceClient =
new InstanceAdminClientImpl(options.getProjectId(), gapicRpc, dbAdminClient);
this.databaseAdminClient = createDatabaseAdminClient();
arpan14 marked this conversation as resolved.
Show resolved Hide resolved
this.instanceAdminClient = createInstanceAdminClient();
}

SpannerImpl(SpannerOptions options) {
Expand Down Expand Up @@ -207,11 +213,21 @@ public DatabaseAdminClient getDatabaseAdminClient() {
return dbAdminClient;
}

@Override
public com.google.cloud.spanner.admin.database.v1.DatabaseAdminClient databaseAdminClient() {
return databaseAdminClient;
}

@Override
public InstanceAdminClient getInstanceAdminClient() {
return instanceClient;
}

@Override
public com.google.cloud.spanner.admin.instance.v1.InstanceAdminClient instanceAdminClient() {
return instanceAdminClient;
}

@Override
public DatabaseClient getDatabaseClient(DatabaseId db) {
synchronized (this) {
Expand Down Expand Up @@ -334,4 +350,24 @@ void setNextPageToken(String nextPageToken) {

abstract S fromProto(T proto);
}

private com.google.cloud.spanner.admin.instance.v1.InstanceAdminClient
createInstanceAdminClient() {
try {
return com.google.cloud.spanner.admin.instance.v1.InstanceAdminClient.create(
gapicRpc.getInstanceAdminStubSettings().createStub());
} catch (IOException ex) {
throw SpannerExceptionFactory.newSpannerException(ex);
}
}

private com.google.cloud.spanner.admin.database.v1.DatabaseAdminClient
createDatabaseAdminClient() {
try {
return com.google.cloud.spanner.admin.database.v1.DatabaseAdminClient.create(
gapicRpc.getDatabaseAdminStubSettings().createStub());
} catch (IOException ex) {
throw SpannerExceptionFactory.newSpannerException(ex);
}
}
}
31 changes: 21 additions & 10 deletions google-cloud-spanner/src/main/java/com/google/cloud/spanner/spi/v1/GapicSpannerRpc.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -243,6 +243,7 @@ public class GapicSpannerRpc implements SpannerRpc {
private final Set<Code> readRetryableCodes;
private final SpannerStub partitionedDmlStub;
private final RetrySettings partitionedDmlRetrySettings;
private final InstanceAdminStubSettings instanceAdminStubSettings;
private final InstanceAdminStub instanceAdminStub;
private final DatabaseAdminStubSettings databaseAdminStubSettings;
private final DatabaseAdminStub databaseAdminStub;
Expand Down Expand Up @@ -435,16 +436,15 @@ public GapicSpannerRpc(final SpannerOptions options) {
.withCheckInterval(pdmlSettings.getStreamWatchdogCheckInterval()));
}
this.partitionedDmlStub = GrpcSpannerStub.create(pdmlSettings.build());

this.instanceAdminStub =
GrpcInstanceAdminStub.create(
options
.getInstanceAdminStubSettings()
.toBuilder()
.setTransportChannelProvider(channelProvider)
.setCredentialsProvider(credentialsProvider)
.setStreamWatchdogProvider(watchdogProvider)
.build());
this.instanceAdminStubSettings =
options
.getInstanceAdminStubSettings()
.toBuilder()
.setTransportChannelProvider(channelProvider)
.setCredentialsProvider(credentialsProvider)
.setStreamWatchdogProvider(watchdogProvider)
.build();
this.instanceAdminStub = GrpcInstanceAdminStub.create(instanceAdminStubSettings);

this.databaseAdminStubSettings =
options
Expand Down Expand Up @@ -510,6 +510,7 @@ public <RequestT, ResponseT> UnaryCallable<RequestT, ResponseT> createUnaryCalla
this.executeQueryRetryableCodes = null;
this.partitionedDmlStub = null;
this.databaseAdminStubSettings = null;
this.instanceAdminStubSettings = null;
this.spannerWatchdog = null;
this.partitionedDmlRetrySettings = null;
}
Expand Down Expand Up @@ -2004,6 +2005,16 @@ public boolean isClosed() {
return rpcIsClosed;
}

@Override
public DatabaseAdminStubSettings getDatabaseAdminStubSettings() {
return databaseAdminStubSettings;
}

@Override
public InstanceAdminStubSettings getInstanceAdminStubSettings() {
return instanceAdminStubSettings;
}

private static final class GrpcStreamingCall implements StreamingCall {
private final ApiCallContext callContext;
private final StreamController controller;
Expand Down
22 changes: 22 additions & 0 deletions google-cloud-spanner/src/main/java/com/google/cloud/spanner/spi/v1/SpannerRpc.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,9 @@
import com.google.cloud.spanner.Restore;
import com.google.cloud.spanner.SpannerException;
import com.google.cloud.spanner.admin.database.v1.stub.DatabaseAdminStub;
import com.google.cloud.spanner.admin.database.v1.stub.DatabaseAdminStubSettings;
import com.google.cloud.spanner.admin.instance.v1.stub.InstanceAdminStub;
import com.google.cloud.spanner.admin.instance.v1.stub.InstanceAdminStubSettings;
import com.google.cloud.spanner.v1.stub.SpannerStubSettings;
import com.google.common.collect.ImmutableList;
import com.google.iam.v1.GetPolicyOptions;
Expand Down Expand Up @@ -500,4 +502,24 @@ TestIamPermissionsResponse testInstanceAdminIAMPermissions(
void shutdown();

boolean isClosed();

/**
* Getter method to obtain the auto-generated instance admin client stub settings.
*
* @return InstanceAdminStubSettings
*/
@InternalApi
default InstanceAdminStubSettings getInstanceAdminStubSettings() {
throw new UnsupportedOperationException("Not implemented");
}

/**
* Getter method to obtain the auto-generated database admin client stub settings.
*
* @return DatabaseAdminStubSettings
*/
@InternalApi
default DatabaseAdminStubSettings getDatabaseAdminStubSettings() {
throw new UnsupportedOperationException("Not implemented");
}
}
Loading
Loading