From 17cb858a63316afc651a4f03bc4c38f9410eb34a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Knut=20Olav=20L=C3=B8ite?= Date: Mon, 1 Jul 2019 21:39:24 +0200 Subject: [PATCH] Spanner: Create new instance if existing Spanner is closed (#5200) * do not hand out a closed Spanner instance SpannerOptions caches any Spanner instance that has been created, and hands this cached instance out to all subsequent calls to SpannerOptions.getService(). This also included closed Spanner instances. The getService() method now returns an error if the Spanner instance has already been closed. * fix small merge error * create a new instance if the service/rpc is closed SpannerOptions.getService() and SpannerOptions.getRpc() should return a new instance instead of throwing an exception if the service/rpc object has been closed. * add test case to ensure correct caching behavior * use shouldRefreshService instead of createNewService * fix merge conflicts * added documentation to shouldRefresh... methods * removed overrides only for comments * fixed naming * added assertions for isClosed() * fixed formatting --- .../java/com/google/cloud/ServiceOptions.java | 20 ++++++++- .../com/google/cloud/spanner/Spanner.java | 3 ++ .../com/google/cloud/spanner/SpannerImpl.java | 5 +++ .../google/cloud/spanner/SpannerOptions.java | 20 +++++++++ .../cloud/spanner/spi/v1/GapicSpannerRpc.java | 7 ++++ .../cloud/spanner/spi/v1/SpannerRpc.java | 2 + .../google/cloud/spanner/SpannerImplTest.java | 42 +++++++++++++++++++ .../cloud/spanner/SpannerOptionsTest.java | 29 +++++++++++++ 8 files changed, 126 insertions(+), 2 deletions(-) diff --git a/google-cloud-clients/google-cloud-core/src/main/java/com/google/cloud/ServiceOptions.java b/google-cloud-clients/google-cloud-core/src/main/java/com/google/cloud/ServiceOptions.java index d08b14e71506..d78958563176 100644 --- a/google-cloud-clients/google-cloud-core/src/main/java/com/google/cloud/ServiceOptions.java +++ b/google-cloud-clients/google-cloud-core/src/main/java/com/google/cloud/ServiceOptions.java @@ -494,24 +494,40 @@ static String getServiceAccountProjectId(String credentialsPath) { */ @SuppressWarnings("unchecked") public ServiceT getService() { - if (service == null) { + if (shouldRefreshService(service)) { service = serviceFactory.create((OptionsT) this); } return service; } + /** + * @param cachedService The currently cached service object + * @return true if the currently cached service object should be refreshed. + */ + protected boolean shouldRefreshService(ServiceT cachedService) { + return cachedService == null; + } + /** * Returns a Service RPC object for the current service. For instance, when using Google Cloud * Storage, it returns a StorageRpc object. */ @SuppressWarnings("unchecked") public ServiceRpc getRpc() { - if (rpc == null) { + if (shouldRefreshRpc(rpc)) { rpc = serviceRpcFactory.create((OptionsT) this); } return rpc; } + /** + * @param cachedRpc The currently cached service object + * @return true if the currently cached service object should be refreshed. + */ + protected boolean shouldRefreshRpc(ServiceRpc cachedRpc) { + return cachedRpc == null; + } + /** * Returns the project ID. Return value can be null (for services that don't require a project * ID). diff --git a/google-cloud-clients/google-cloud-spanner/src/main/java/com/google/cloud/spanner/Spanner.java b/google-cloud-clients/google-cloud-spanner/src/main/java/com/google/cloud/spanner/Spanner.java index 75c062f96811..0c6bec4ea830 100644 --- a/google-cloud-clients/google-cloud-spanner/src/main/java/com/google/cloud/spanner/Spanner.java +++ b/google-cloud-clients/google-cloud-spanner/src/main/java/com/google/cloud/spanner/Spanner.java @@ -105,4 +105,7 @@ public interface Spanner extends Service, AutoCloseable { */ @Override void close(); + + /** @return true if this {@link Spanner} object is closed. */ + boolean isClosed(); } diff --git a/google-cloud-clients/google-cloud-spanner/src/main/java/com/google/cloud/spanner/SpannerImpl.java b/google-cloud-clients/google-cloud-spanner/src/main/java/com/google/cloud/spanner/SpannerImpl.java index 337326f5ca31..42852fee2a7d 100644 --- a/google-cloud-clients/google-cloud-spanner/src/main/java/com/google/cloud/spanner/SpannerImpl.java +++ b/google-cloud-clients/google-cloud-spanner/src/main/java/com/google/cloud/spanner/SpannerImpl.java @@ -244,6 +244,11 @@ public void close() { } } + @Override + public boolean isClosed() { + return spannerIsClosed; + } + /** * Encapsulates state to be passed to the {@link SpannerRpc} layer for a given session. Currently * used to select the {@link io.grpc.Channel} to be used in issuing the RPCs in a Session. diff --git a/google-cloud-clients/google-cloud-spanner/src/main/java/com/google/cloud/spanner/SpannerOptions.java b/google-cloud-clients/google-cloud-spanner/src/main/java/com/google/cloud/spanner/SpannerOptions.java index cf3e7676b44e..a1f306372331 100644 --- a/google-cloud-clients/google-cloud-spanner/src/main/java/com/google/cloud/spanner/SpannerOptions.java +++ b/google-cloud-clients/google-cloud-spanner/src/main/java/com/google/cloud/spanner/SpannerOptions.java @@ -436,6 +436,26 @@ protected SpannerRpc getSpannerRpcV1() { return (SpannerRpc) getRpc(); } + /** + * @return true if the cached Spanner service instance is null or + * closed. This will cause the method {@link #getService()} to create a new {@link SpannerRpc} + * instance when one is requested. + */ + @Override + protected boolean shouldRefreshService(Spanner cachedService) { + return cachedService == null || cachedService.isClosed(); + } + + /** + * @return true if the cached {@link ServiceRpc} instance is null or + * closed. This will cause the method {@link #getRpc()} to create a new {@link Spanner} + * instance when one is requested. + */ + @Override + protected boolean shouldRefreshRpc(ServiceRpc cachedRpc) { + return cachedRpc == null || ((SpannerRpc) cachedRpc).isClosed(); + } + @SuppressWarnings("unchecked") @Override public Builder toBuilder() { diff --git a/google-cloud-clients/google-cloud-spanner/src/main/java/com/google/cloud/spanner/spi/v1/GapicSpannerRpc.java b/google-cloud-clients/google-cloud-spanner/src/main/java/com/google/cloud/spanner/spi/v1/GapicSpannerRpc.java index a6ca0e003ac5..e309f60ab8a4 100644 --- a/google-cloud-clients/google-cloud-spanner/src/main/java/com/google/cloud/spanner/spi/v1/GapicSpannerRpc.java +++ b/google-cloud-clients/google-cloud-spanner/src/main/java/com/google/cloud/spanner/spi/v1/GapicSpannerRpc.java @@ -159,6 +159,7 @@ private synchronized void shutdown() { private static final int DEFAULT_PERIOD_SECONDS = 10; private final ManagedInstantiatingExecutorProvider executorProvider; + private boolean rpcIsClosed; private final SpannerStub spannerStub; private final InstanceAdminStub instanceAdminStub; private final DatabaseAdminStub databaseAdminStub; @@ -600,6 +601,7 @@ private GrpcCallContext newCallContext(@Nullable Map options, String @Override public void shutdown() { + this.rpcIsClosed = true; this.spannerStub.close(); this.instanceAdminStub.close(); this.databaseAdminStub.close(); @@ -607,6 +609,11 @@ public void shutdown() { this.executorProvider.shutdown(); } + @Override + public boolean isClosed() { + return rpcIsClosed; + } + /** * A {@code ResponseObserver} that exposes the {@code StreamController} and delegates callbacks to * the {@link ResultStreamConsumer}. diff --git a/google-cloud-clients/google-cloud-spanner/src/main/java/com/google/cloud/spanner/spi/v1/SpannerRpc.java b/google-cloud-clients/google-cloud-spanner/src/main/java/com/google/cloud/spanner/spi/v1/SpannerRpc.java index 500f369f67a8..593ba3c5ec06 100644 --- a/google-cloud-clients/google-cloud-spanner/src/main/java/com/google/cloud/spanner/spi/v1/SpannerRpc.java +++ b/google-cloud-clients/google-cloud-spanner/src/main/java/com/google/cloud/spanner/spi/v1/SpannerRpc.java @@ -233,4 +233,6 @@ PartitionResponse partitionRead(PartitionReadRequest request, @Nullable Map