-
Notifications
You must be signed in to change notification settings - Fork 123
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.
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: support public methods to use autogenerated admin clients. #2878
Conversation
…od while retrying exceptions in unit tests. * For details on issue see - googleapis#2206
fix: prevent illegal negative timeout values into thread sleep() method while retrying exceptions in unit tests.
google-cloud-spanner/src/main/java/com/google/cloud/spanner/Spanner.java
Outdated
Show resolved
Hide resolved
google-cloud-spanner/src/main/java/com/google/cloud/spanner/Spanner.java
Outdated
Show resolved
Hide resolved
google-cloud-spanner/src/main/java/com/google/cloud/spanner/Spanner.java
Outdated
Show resolved
Hide resolved
google-cloud-spanner/src/main/java/com/google/cloud/spanner/Spanner.java
Outdated
Show resolved
Hide resolved
google-cloud-spanner/src/main/java/com/google/cloud/spanner/Spanner.java
Outdated
Show resolved
Hide resolved
google-cloud-spanner/src/main/java/com/google/cloud/spanner/Spanner.java
Outdated
Show resolved
Hide resolved
* }</pre> | ||
* <!--SNIPPET get_dbadmin_client--> | ||
*/ | ||
default com.google.cloud.spanner.admin.database.v1.DatabaseAdminClient databaseAdminClient() { |
There was a problem hiding this comment.
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:
- We document that users should never call
close()
on the instance that is returned. - OR: Create a new stub for each new instance that is returned by this method.
- 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 theSpanner
instance that created it.
I think that option 3 is the safest option.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
- Creating a
ManagedDatabaseAdminClient
which extends theDatabaseAdminClient
class and overrides theclose()
method. This is not possible to implement since the method is marked as final and hence we cannot override it. - 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.
There was a problem hiding this comment.
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.
google-cloud-spanner/src/main/java/com/google/cloud/spanner/SpannerImpl.java
Outdated
Show resolved
Hide resolved
…anner.java Co-authored-by: Knut Olav Løite <[email protected]>
…anner.java Co-authored-by: Knut Olav Løite <[email protected]>
…anner.java Co-authored-by: Knut Olav Løite <[email protected]>
…anner.java Co-authored-by: Knut Olav Løite <[email protected]>
google-cloud-spanner/src/main/java/com/google/cloud/spanner/SpannerImpl.java
Outdated
Show resolved
Hide resolved
google-cloud-spanner/src/main/java/com/google/cloud/spanner/Spanner.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, assuming that the emulator tests succeed (the failure seemed completely unrelated).
Also one minor request: The pull request description needs to be updated to reflect the latest method names and how these work.
🤖 I have created a release *beep* *boop* --- ## [6.59.0](https://togithub.com/googleapis/java-spanner/compare/v6.58.0...v6.59.0) (2024-02-15) ### Features * Support public methods to use autogenerated admin clients. ([#2878](https://togithub.com/googleapis/java-spanner/issues/2878)) ([53bcb3e](https://togithub.com/googleapis/java-spanner/commit/53bcb3eca2e814472c3def24e8e03d47652a8e42)) ### Dependencies * Update dependency com.google.cloud:sdk-platform-java-config to v3.25.0 ([#2888](https://togithub.com/googleapis/java-spanner/issues/2888)) ([8e2da51](https://togithub.com/googleapis/java-spanner/commit/8e2da5126263c7acd134fb7fcfeb590ca190ce8e)) ### Documentation * README for OpenTelemetry metrics and traces ([#2880](https://togithub.com/googleapis/java-spanner/issues/2880)) ([c8632f5](https://togithub.com/googleapis/java-spanner/commit/c8632f5b2f462420a8c2a1f4308a68a18a414472)) * Samples and tests for database Admin APIs. ([#2775](https://togithub.com/googleapis/java-spanner/issues/2775)) ([14ae01c](https://togithub.com/googleapis/java-spanner/commit/14ae01cd82e455a0dc22d7e3bb8c362e541ede12)) --- This PR was generated with [Release Please](https://togithub.com/googleapis/release-please). See [documentation](https://togithub.com/googleapis/release-please#release-please).
Supporting two new methods
createInstanceAdminClient()
andcreateDatabaseAdminClient
in the Spanner public interface for customers to use the upgraded admin clients.Sample Usages
Notes:
SPANNER_EMULATOR_HOST
.