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.

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

Merged
merged 34 commits into from
Feb 13, 2024

Conversation

arpan14
Copy link
Contributor

@arpan14 arpan14 commented Feb 10, 2024

Supporting two new methods createInstanceAdminClient() and createDatabaseAdminClient in the Spanner public interface for customers to use the upgraded admin clients.

Sample Usages

SpannerOptions options = SpannerOptions.newBuilder().build();
Spanner spanner = options.getService();
DatabaseAdminClient dbAdminClient = spanner.createInstanceAdminClient();   
SpannerOptions options = SpannerOptions.newBuilder().build();
Spanner spanner = options.getService();
InstanceAdminClient instanceAdminClient = spanner.createDatabaseAdminClient();

Notes:

  • The new public methods support all existing setting (set via environment variables). For ex - Emulator can be used in the same way as before by setting env variable SPANNER_EMULATOR_HOST.

@arpan14 arpan14 requested a review from a team as a code owner February 10, 2024 20:03
@product-auto-label product-auto-label bot added size: l Pull request size is large. api: spanner Issues related to the googleapis/java-spanner API. labels Feb 10, 2024
@arpan14 arpan14 changed the title feat: support emulator with autogenerated admin clients. feat: support public methods to use autogenerated admin clients. Feb 10, 2024
@arpan14 arpan14 requested a review from a team as a code owner February 12, 2024 06:36
* }</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
Contributor Author

@arpan14 arpan14 Feb 12, 2024

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
Contributor 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
Contributor 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.

@arpan14 arpan14 requested a review from olavloite February 13, 2024 09:51
@arpan14 arpan14 added the owlbot:run Add this label to trigger the Owlbot post processor. label Feb 13, 2024
@gcf-owl-bot gcf-owl-bot bot removed the owlbot:run Add this label to trigger the Owlbot post processor. label Feb 13, 2024
Copy link
Collaborator

@olavloite olavloite left a 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.

@arpan14 arpan14 merged commit 53bcb3e into googleapis:main Feb 13, 2024
24 of 25 checks passed
@arpan14 arpan14 deleted the admin-emulator-support branch February 13, 2024 12:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: spanner Issues related to the googleapis/java-spanner API. size: l Pull request size is large.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants