From 4a89f892b92b7e5c546325755783dcf80c2a9b94 Mon Sep 17 00:00:00 2001 From: Olav Loite Date: Wed, 15 May 2019 08:56:17 +0200 Subject: [PATCH 01/11] 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. --- .../com/google/cloud/spanner/Spanner.java | 3 + .../com/google/cloud/spanner/SpannerImpl.java | 5 + .../google/cloud/spanner/SpannerOptions.java | 30 +++++ .../cloud/spanner/spi/v1/GapicSpannerRpc.java | 7 ++ .../cloud/spanner/spi/v1/SpannerRpc.java | 2 + .../google/cloud/spanner/SpannerImplTest.java | 108 ++++++++++++++++++ .../spanner/spi/v1/GapicSpannerRpcTest.java | 1 + 7 files changed, 156 insertions(+) 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 10672da94083..e5db006144e1 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; + } + /** * Checks that the current context is still valid, throwing a CANCELLED or DEADLINE_EXCEEDED error * if not. 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..01a7fdaefe3d 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,36 @@ protected SpannerRpc getSpannerRpcV1() { return (SpannerRpc) getRpc(); } + /** + * Returns a {@link Spanner} service object. Note that only the first call to this method will + * create a new instance, and all subsequent calls to this method will return the same instance. + * Calling this method after the {@link Spanner} instance has been closed will cause an exception. + */ + @Override + public Spanner getService() { + Spanner spanner = super.getService(); + if (spanner.isClosed()) { + throw new IllegalStateException( + "The Spanner service of this SpannerOptions has been closed. Create a new SpannerOptions instance and call getService() on the new instance instead."); + } + return spanner; + } + + /** + * Returns a {@link SpannerRpc} object. Note that only the first call to this method will create a + * new instance, and all subsequent calls to this method will return the same instance. Calling + * this method after the instance has been closed will cause an exception. + */ + @Override + public ServiceRpc getRpc() { + SpannerRpc rpc = (SpannerRpc) super.getRpc(); + if (rpc.isClosed()) { + throw new IllegalStateException( + "The Spanner service of this SpannerOptions has been closed. Create a new SpannerOptions instance and call getRpc() on the new instance instead."); + } + return rpc; + } + @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() { + @Override + public Void call() throws Exception { + throw new Exception("Should be translated to SpannerException"); + } + }); + } catch (SpannerException e) { + assertThat(e.getErrorCode()).isEqualTo(ErrorCode.INTERNAL); + assertThat(e.getMessage().contains("Unexpected exception thrown")); + } + } + + @Test + public void sslHandshakeExceptionIsNotRetryable() { + // Verify that a SpannerException with code UNAVAILABLE and cause SSLHandshakeException is not + // retryable. + boolean gotExpectedException = false; + try { + SpannerImpl.runWithRetries( + new Callable() { + @Override + public Void call() throws Exception { + throw SpannerExceptionFactory.newSpannerException( + ErrorCode.UNAVAILABLE, + "This exception should not be retryable", + new SSLHandshakeException("some SSL handshake exception")); + } + }); + } catch (SpannerException e) { + gotExpectedException = true; + assertThat(e.isRetryable(), is(false)); + assertThat(e.getErrorCode()).isEqualTo(ErrorCode.UNAVAILABLE); + assertThat(e.getMessage().contains("This exception should not be retryable")); + } + assertThat(gotExpectedException, is(true)); + + // Verify that any other SpannerException with code UNAVAILABLE is retryable. + SpannerImpl.runWithRetries( + new Callable() { + private boolean firstTime = true; + + @Override + public Void call() throws Exception { + // Keep track of whethr this is the first call or a subsequent call to avoid an infinite + // loop. + if (firstTime) { + firstTime = false; + throw SpannerExceptionFactory.newSpannerException( + ErrorCode.UNAVAILABLE, + "This exception should be retryable", + new Exception("some other exception")); + } + return null; + } + }); + } + + @Test + public void testSpannerClosed() throws InterruptedException { + SpannerOptions options = createSpannerOptions(); + Spanner spanner1 = options.getService(); + Spanner spanner2 = options.getService(); + ServiceRpc rpc1 = options.getRpc(); + ServiceRpc rpc2 = options.getRpc(); + // The SpannerOptions object should return the same instance. + assertThat(spanner1 == spanner2, is(true)); + assertThat(rpc1 == rpc2, is(true)); + spanner1.close(); + // The SpannerOptions should now no longer be valid. + try { + options.getService(); + fail("missing expected exception"); + } catch (IllegalStateException e) { + // This is the expected exception. + } + // The SpannerOptions should now no longer be valid. + try { + options.getRpc(); + fail("missing expected exception"); + } catch (IllegalStateException e) { + // This is the expected exception. + } + // Creating a copy of the SpannerOptions should result in new instances. + options = options.toBuilder().build(); + spanner1 = options.getService(); + rpc1 = options.getRpc(); + assertThat(spanner1 == spanner2, is(false)); + assertThat(rpc1 == rpc2, is(false)); + spanner2 = options.getService(); + rpc2 = options.getRpc(); + assertThat(spanner1 == spanner2, is(true)); + assertThat(rpc1 == rpc2, is(true)); + spanner1.close(); + } + + private SpannerOptions createSpannerOptions() { + return SpannerOptions.newBuilder() + .setProjectId("[PROJECT]") + .setCredentials(NoCredentials.getInstance()) + .build(); + } } diff --git a/google-cloud-clients/google-cloud-spanner/src/test/java/com/google/cloud/spanner/spi/v1/GapicSpannerRpcTest.java b/google-cloud-clients/google-cloud-spanner/src/test/java/com/google/cloud/spanner/spi/v1/GapicSpannerRpcTest.java index bbf53b0cf0cc..75f49d451dcb 100644 --- a/google-cloud-clients/google-cloud-spanner/src/test/java/com/google/cloud/spanner/spi/v1/GapicSpannerRpcTest.java +++ b/google-cloud-clients/google-cloud-spanner/src/test/java/com/google/cloud/spanner/spi/v1/GapicSpannerRpcTest.java @@ -167,6 +167,7 @@ public void testCloseAllThreadsWhenClosingSpanner() throws InterruptedException DatabaseAdminClient databaseAdminClient = spanner.getDatabaseAdminClient(); databaseAdminClient.getDatabase("projects/[PROJECT]/instances/[INSTANCE]", "[DATABASE]"); } + // Now close the Spanner instance and check whether the threads are shutdown or not. spanner.close(); // Wait for up to two seconds to allow the threads to actually shutdown. From 599535fa7c1719e65846b58f9d9e98f4884486d5 Mon Sep 17 00:00:00 2001 From: Olav Loite Date: Wed, 15 May 2019 16:44:35 +0200 Subject: [PATCH 02/11] fix small merge error --- .../com/google/cloud/spanner/spi/v1/GapicSpannerRpcTest.java | 1 - 1 file changed, 1 deletion(-) diff --git a/google-cloud-clients/google-cloud-spanner/src/test/java/com/google/cloud/spanner/spi/v1/GapicSpannerRpcTest.java b/google-cloud-clients/google-cloud-spanner/src/test/java/com/google/cloud/spanner/spi/v1/GapicSpannerRpcTest.java index 75f49d451dcb..bbf53b0cf0cc 100644 --- a/google-cloud-clients/google-cloud-spanner/src/test/java/com/google/cloud/spanner/spi/v1/GapicSpannerRpcTest.java +++ b/google-cloud-clients/google-cloud-spanner/src/test/java/com/google/cloud/spanner/spi/v1/GapicSpannerRpcTest.java @@ -167,7 +167,6 @@ public void testCloseAllThreadsWhenClosingSpanner() throws InterruptedException DatabaseAdminClient databaseAdminClient = spanner.getDatabaseAdminClient(); databaseAdminClient.getDatabase("projects/[PROJECT]/instances/[INSTANCE]", "[DATABASE]"); } - // Now close the Spanner instance and check whether the threads are shutdown or not. spanner.close(); // Wait for up to two seconds to allow the threads to actually shutdown. From 9bbd0e2c3b7837756776f702af99518969b8aef1 Mon Sep 17 00:00:00 2001 From: Olav Loite Date: Sat, 18 May 2019 08:26:02 +0200 Subject: [PATCH 03/11] 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. --- .../java/com/google/cloud/ServiceOptions.java | 20 ++++++++++ .../google/cloud/spanner/SpannerOptions.java | 20 +++++----- .../google/cloud/spanner/SpannerImplTest.java | 39 ++++++++----------- 3 files changed, 46 insertions(+), 33 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..c21633250db0 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 @@ -500,6 +500,16 @@ public ServiceT getService() { return service; } + /** + * Creates a new service object and returns it. Subsequent calls to {@link #getService()} will + * return this instance. + */ + @SuppressWarnings("unchecked") + protected ServiceT createNewService() { + service = serviceFactory.create((OptionsT) this); + return service; + } + /** * Returns a Service RPC object for the current service. For instance, when using Google Cloud * Storage, it returns a StorageRpc object. @@ -512,6 +522,16 @@ public ServiceRpc getRpc() { return rpc; } + /** + * Creates a new rpc object and returns it. Subsequent calls to {@link #getRpc()} will return this + * instance. + */ + @SuppressWarnings("unchecked") + protected ServiceRpc createNewRpc() { + rpc = serviceRpcFactory.create((OptionsT) this); + return rpc; + } + /** * 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/SpannerOptions.java b/google-cloud-clients/google-cloud-spanner/src/main/java/com/google/cloud/spanner/SpannerOptions.java index 01a7fdaefe3d..0cabe660d02b 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 @@ -437,31 +437,31 @@ protected SpannerRpc getSpannerRpcV1() { } /** - * Returns a {@link Spanner} service object. Note that only the first call to this method will - * create a new instance, and all subsequent calls to this method will return the same instance. - * Calling this method after the {@link Spanner} instance has been closed will cause an exception. + * Returns a {@link Spanner} service object. This method will create a {@link Spanner} instance on + * the first call and subsequent calls to this method will return the same {@link Spanner} + * instance. If the instance is closed, the following call to this method will create a new {@link + * Spanner} instance. */ @Override public Spanner getService() { Spanner spanner = super.getService(); if (spanner.isClosed()) { - throw new IllegalStateException( - "The Spanner service of this SpannerOptions has been closed. Create a new SpannerOptions instance and call getService() on the new instance instead."); + spanner = createNewService(); } return spanner; } /** - * Returns a {@link SpannerRpc} object. Note that only the first call to this method will create a - * new instance, and all subsequent calls to this method will return the same instance. Calling - * this method after the instance has been closed will cause an exception. + * Returns a {@link SpannerRpc} object. This method will create a {@link ServiceRpc} instance on + * the first call and subsequent calls to this method will return the same {@link ServiceRpc} + * instance. If the instance is closed, the following call to this method will create a new {@link + * ServiceRpc} instance. */ @Override public ServiceRpc getRpc() { SpannerRpc rpc = (SpannerRpc) super.getRpc(); if (rpc.isClosed()) { - throw new IllegalStateException( - "The Spanner service of this SpannerOptions has been closed. Create a new SpannerOptions instance and call getRpc() on the new instance instead."); + rpc = (SpannerRpc) createNewRpc(); } return rpc; } diff --git a/google-cloud-clients/google-cloud-spanner/src/test/java/com/google/cloud/spanner/SpannerImplTest.java b/google-cloud-clients/google-cloud-spanner/src/test/java/com/google/cloud/spanner/SpannerImplTest.java index aa5404266fd0..9247cc982423 100644 --- a/google-cloud-clients/google-cloud-spanner/src/test/java/com/google/cloud/spanner/SpannerImplTest.java +++ b/google-cloud-clients/google-cloud-spanner/src/test/java/com/google/cloud/spanner/SpannerImplTest.java @@ -197,31 +197,24 @@ public void testSpannerClosed() throws InterruptedException { assertThat(spanner1 == spanner2, is(true)); assertThat(rpc1 == rpc2, is(true)); spanner1.close(); - // The SpannerOptions should now no longer be valid. - try { - options.getService(); - fail("missing expected exception"); - } catch (IllegalStateException e) { - // This is the expected exception. - } - // The SpannerOptions should now no longer be valid. - try { - options.getRpc(); - fail("missing expected exception"); - } catch (IllegalStateException e) { - // This is the expected exception. - } + // A new instance should be returned as the Spanner instance has been closed. + Spanner spanner3 = options.getService(); + assertThat(spanner1 == spanner3, is(false)); + // A new instance should be returned as the Spanner instance has been closed. + ServiceRpc rpc3 = options.getRpc(); + assertThat(rpc1 == rpc3, is(false)); // Creating a copy of the SpannerOptions should result in new instances. options = options.toBuilder().build(); - spanner1 = options.getService(); - rpc1 = options.getRpc(); - assertThat(spanner1 == spanner2, is(false)); - assertThat(rpc1 == rpc2, is(false)); - spanner2 = options.getService(); - rpc2 = options.getRpc(); - assertThat(spanner1 == spanner2, is(true)); - assertThat(rpc1 == rpc2, is(true)); - spanner1.close(); + Spanner spanner4 = options.getService(); + ServiceRpc rpc4 = options.getRpc(); + assertThat(spanner4 == spanner3, is(false)); + assertThat(rpc4 == rpc3, is(false)); + Spanner spanner5 = options.getService(); + ServiceRpc rpc5 = options.getRpc(); + assertThat(spanner4 == spanner5, is(true)); + assertThat(rpc4 == rpc5, is(true)); + spanner3.close(); + spanner4.close(); } private SpannerOptions createSpannerOptions() { From 5561b8e284362f983034c7842d10da88db568e77 Mon Sep 17 00:00:00 2001 From: Olav Loite Date: Fri, 14 Jun 2019 07:55:31 +0200 Subject: [PATCH 04/11] add test case to ensure correct caching behavior --- .../cloud/spanner/SpannerOptionsTest.java | 24 +++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/google-cloud-clients/google-cloud-spanner/src/test/java/com/google/cloud/spanner/SpannerOptionsTest.java b/google-cloud-clients/google-cloud-spanner/src/test/java/com/google/cloud/spanner/SpannerOptionsTest.java index dd28ca902808..867699685819 100644 --- a/google-cloud-clients/google-cloud-spanner/src/test/java/com/google/cloud/spanner/SpannerOptionsTest.java +++ b/google-cloud-clients/google-cloud-spanner/src/test/java/com/google/cloud/spanner/SpannerOptionsTest.java @@ -17,10 +17,13 @@ package com.google.cloud.spanner; import static com.google.common.truth.Truth.assertThat; +import static org.hamcrest.CoreMatchers.is; +import static org.junit.Assert.assertThat; import com.google.api.gax.retrying.RetrySettings; import com.google.api.gax.rpc.ServerStreamingCallSettings; import com.google.api.gax.rpc.UnaryCallSettings; +import com.google.cloud.NoCredentials; import com.google.cloud.TransportOptions; import com.google.cloud.spanner.admin.database.v1.stub.DatabaseAdminStubSettings; import com.google.cloud.spanner.admin.instance.v1.stub.InstanceAdminStubSettings; @@ -353,4 +356,25 @@ public void testNullSessionLabels() { thrown.expect(NullPointerException.class); SpannerOptions.newBuilder().setSessionLabels(null); } + + @Test + public void testDoNotCacheClosedSpannerInstance() { + SpannerOptions options = SpannerOptions.newBuilder() + .setProjectId("[PROJECT]") + .setCredentials(NoCredentials.getInstance()) + .build(); + // Getting a service twice should give the same instance. + Spanner service1 = options.getService(); + Spanner service2 = options.getService(); + assertThat(service1 == service2, is(true)); + // Closing a service instance should cause the SpannerOptions to create a new service. + service1.close(); + Spanner service3 = options.getService(); + assertThat(service3 == service1, is(false)); + assertThat(service3.isClosed(), is(false)); + // Getting another service from the SpannerOptions should return the new cached instance. + Spanner service4 = options.getService(); + assertThat(service3 == service4, is(true)); + service3.close(); + } } From d273479077b833f62f983af0daf34db73749220e Mon Sep 17 00:00:00 2001 From: Olav Loite Date: Sat, 15 Jun 2019 08:19:58 +0200 Subject: [PATCH 05/11] use shouldRefreshService instead of createNewService --- .../java/com/google/cloud/ServiceOptions.java | 24 +++++++--------- .../google/cloud/spanner/SpannerOptions.java | 28 +++++++++++-------- .../cloud/spanner/SpannerOptionsTest.java | 9 +++--- 3 files changed, 31 insertions(+), 30 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 c21633250db0..56933b12f18c 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,20 +494,18 @@ static String getServiceAccountProjectId(String credentialsPath) { */ @SuppressWarnings("unchecked") public ServiceT getService() { - if (service == null) { + if (shouldRefreshService(service)) { service = serviceFactory.create((OptionsT) this); } return service; } /** - * Creates a new service object and returns it. Subsequent calls to {@link #getService()} will - * return this instance. + * @param cachedService The currently cached service object + * @return true if the currently cached service object should be refreshed. */ - @SuppressWarnings("unchecked") - protected ServiceT createNewService() { - service = serviceFactory.create((OptionsT) this); - return service; + protected boolean shouldRefreshService(ServiceT cachedService) { + return cachedService == null; } /** @@ -516,20 +514,18 @@ protected ServiceT createNewService() { */ @SuppressWarnings("unchecked") public ServiceRpc getRpc() { - if (rpc == null) { + if (shouldRefreshRpc(rpc)) { rpc = serviceRpcFactory.create((OptionsT) this); } return rpc; } /** - * Creates a new rpc object and returns it. Subsequent calls to {@link #getRpc()} will return this - * instance. + * @param cachedService The currently cached service object + * @return true if the currently cached service object should be refreshed. */ - @SuppressWarnings("unchecked") - protected ServiceRpc createNewRpc() { - rpc = serviceRpcFactory.create((OptionsT) this); - return rpc; + protected boolean shouldRefreshRpc(ServiceRpc cachedRpc) { + return cachedRpc == null; } /** 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 0cabe660d02b..565821a56c64 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 @@ -433,7 +433,7 @@ public Set getScopes() { } protected SpannerRpc getSpannerRpcV1() { - return (SpannerRpc) getRpc(); + return getRpc(); } /** @@ -444,11 +444,13 @@ protected SpannerRpc getSpannerRpcV1() { */ @Override public Spanner getService() { - Spanner spanner = super.getService(); - if (spanner.isClosed()) { - spanner = createNewService(); - } - return spanner; + // Method is only overridden in order to supply additional documentation. + return super.getService(); + } + + @Override + protected boolean shouldRefreshService(Spanner cachedService) { + return cachedService == null || cachedService.isClosed(); } /** @@ -458,12 +460,14 @@ public Spanner getService() { * ServiceRpc} instance. */ @Override - public ServiceRpc getRpc() { - SpannerRpc rpc = (SpannerRpc) super.getRpc(); - if (rpc.isClosed()) { - rpc = (SpannerRpc) createNewRpc(); - } - return rpc; + public SpannerRpc getRpc() { + // Method is only overridden in order to supply additional documentation. + return (SpannerRpc) super.getRpc(); + } + + @Override + protected boolean shouldRefreshRpc(ServiceRpc cachedRpc) { + return cachedRpc == null || ((SpannerRpc) cachedRpc).isClosed(); } @SuppressWarnings("unchecked") diff --git a/google-cloud-clients/google-cloud-spanner/src/test/java/com/google/cloud/spanner/SpannerOptionsTest.java b/google-cloud-clients/google-cloud-spanner/src/test/java/com/google/cloud/spanner/SpannerOptionsTest.java index 867699685819..a667b7385ae2 100644 --- a/google-cloud-clients/google-cloud-spanner/src/test/java/com/google/cloud/spanner/SpannerOptionsTest.java +++ b/google-cloud-clients/google-cloud-spanner/src/test/java/com/google/cloud/spanner/SpannerOptionsTest.java @@ -359,10 +359,11 @@ public void testNullSessionLabels() { @Test public void testDoNotCacheClosedSpannerInstance() { - SpannerOptions options = SpannerOptions.newBuilder() - .setProjectId("[PROJECT]") - .setCredentials(NoCredentials.getInstance()) - .build(); + SpannerOptions options = + SpannerOptions.newBuilder() + .setProjectId("[PROJECT]") + .setCredentials(NoCredentials.getInstance()) + .build(); // Getting a service twice should give the same instance. Spanner service1 = options.getService(); Spanner service2 = options.getService(); From fb2a4db5b97276bbc36b2e7078c6c5fadfc8d558 Mon Sep 17 00:00:00 2001 From: Olav Loite Date: Sat, 15 Jun 2019 13:01:03 +0200 Subject: [PATCH 06/11] fix merge conflicts --- .../google/cloud/spanner/SpannerImplTest.java | 63 +------------------ 1 file changed, 2 insertions(+), 61 deletions(-) diff --git a/google-cloud-clients/google-cloud-spanner/src/test/java/com/google/cloud/spanner/SpannerImplTest.java b/google-cloud-clients/google-cloud-spanner/src/test/java/com/google/cloud/spanner/SpannerImplTest.java index 9247cc982423..6a6051fc8747 100644 --- a/google-cloud-clients/google-cloud-spanner/src/test/java/com/google/cloud/spanner/SpannerImplTest.java +++ b/google-cloud-clients/google-cloud-spanner/src/test/java/com/google/cloud/spanner/SpannerImplTest.java @@ -17,6 +17,8 @@ package com.google.cloud.spanner; import static com.google.common.truth.Truth.assertThat; +import static org.hamcrest.CoreMatchers.is; +import static org.junit.Assert.assertThat; import static org.junit.Assert.fail; import static org.mockito.Mockito.when; @@ -125,67 +127,6 @@ public void getDbclientAfterCloseThrows() { } } - @Test - public void exceptionIsTranslated() { - try { - SpannerImpl.runWithRetries( - new Callable() { - @Override - public Void call() throws Exception { - throw new Exception("Should be translated to SpannerException"); - } - }); - } catch (SpannerException e) { - assertThat(e.getErrorCode()).isEqualTo(ErrorCode.INTERNAL); - assertThat(e.getMessage().contains("Unexpected exception thrown")); - } - } - - @Test - public void sslHandshakeExceptionIsNotRetryable() { - // Verify that a SpannerException with code UNAVAILABLE and cause SSLHandshakeException is not - // retryable. - boolean gotExpectedException = false; - try { - SpannerImpl.runWithRetries( - new Callable() { - @Override - public Void call() throws Exception { - throw SpannerExceptionFactory.newSpannerException( - ErrorCode.UNAVAILABLE, - "This exception should not be retryable", - new SSLHandshakeException("some SSL handshake exception")); - } - }); - } catch (SpannerException e) { - gotExpectedException = true; - assertThat(e.isRetryable(), is(false)); - assertThat(e.getErrorCode()).isEqualTo(ErrorCode.UNAVAILABLE); - assertThat(e.getMessage().contains("This exception should not be retryable")); - } - assertThat(gotExpectedException, is(true)); - - // Verify that any other SpannerException with code UNAVAILABLE is retryable. - SpannerImpl.runWithRetries( - new Callable() { - private boolean firstTime = true; - - @Override - public Void call() throws Exception { - // Keep track of whethr this is the first call or a subsequent call to avoid an infinite - // loop. - if (firstTime) { - firstTime = false; - throw SpannerExceptionFactory.newSpannerException( - ErrorCode.UNAVAILABLE, - "This exception should be retryable", - new Exception("some other exception")); - } - return null; - } - }); - } - @Test public void testSpannerClosed() throws InterruptedException { SpannerOptions options = createSpannerOptions(); From 5fbb9125142147dec7ecca3c266ba96974b1ff46 Mon Sep 17 00:00:00 2001 From: Olav Loite Date: Mon, 17 Jun 2019 19:04:31 +0200 Subject: [PATCH 07/11] added documentation to shouldRefresh... methods --- .../java/com/google/cloud/spanner/SpannerOptions.java | 10 ++++++++++ 1 file changed, 10 insertions(+) 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 565821a56c64..ff3f4fbabd08 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 @@ -448,6 +448,11 @@ public Spanner getService() { return super.getService(); } + /** + * @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(); @@ -465,6 +470,11 @@ public SpannerRpc getRpc() { return (SpannerRpc) super.getRpc(); } + /** + * @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(); From 7692138cf255c7f93151c6b0d566bb8e243f99e3 Mon Sep 17 00:00:00 2001 From: Olav Loite Date: Fri, 21 Jun 2019 10:03:52 +0200 Subject: [PATCH 08/11] removed overrides only for comments --- .../google/cloud/spanner/SpannerOptions.java | 26 +------------------ 1 file changed, 1 insertion(+), 25 deletions(-) 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 ff3f4fbabd08..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 @@ -433,19 +433,7 @@ public Set getScopes() { } protected SpannerRpc getSpannerRpcV1() { - return getRpc(); - } - - /** - * Returns a {@link Spanner} service object. This method will create a {@link Spanner} instance on - * the first call and subsequent calls to this method will return the same {@link Spanner} - * instance. If the instance is closed, the following call to this method will create a new {@link - * Spanner} instance. - */ - @Override - public Spanner getService() { - // Method is only overridden in order to supply additional documentation. - return super.getService(); + return (SpannerRpc) getRpc(); } /** @@ -458,18 +446,6 @@ protected boolean shouldRefreshService(Spanner cachedService) { return cachedService == null || cachedService.isClosed(); } - /** - * Returns a {@link SpannerRpc} object. This method will create a {@link ServiceRpc} instance on - * the first call and subsequent calls to this method will return the same {@link ServiceRpc} - * instance. If the instance is closed, the following call to this method will create a new {@link - * ServiceRpc} instance. - */ - @Override - public SpannerRpc getRpc() { - // Method is only overridden in order to supply additional documentation. - return (SpannerRpc) super.getRpc(); - } - /** * @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} From b8bf65131de0514736c599588d12a45d0f37ec46 Mon Sep 17 00:00:00 2001 From: Olav Loite Date: Wed, 26 Jun 2019 15:31:40 +0200 Subject: [PATCH 09/11] fixed naming --- .../src/main/java/com/google/cloud/ServiceOptions.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 56933b12f18c..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 @@ -521,7 +521,7 @@ public ServiceRpc getRpc() { } /** - * @param cachedService The currently cached service object + * @param cachedRpc The currently cached service object * @return true if the currently cached service object should be refreshed. */ protected boolean shouldRefreshRpc(ServiceRpc cachedRpc) { From 2cc0635055dc9f32699794dbd53462800a2e127b Mon Sep 17 00:00:00 2001 From: Olav Loite Date: Wed, 26 Jun 2019 15:32:05 +0200 Subject: [PATCH 10/11] added assertions for isClosed() --- .../java/com/google/cloud/spanner/SpannerOptionsTest.java | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/google-cloud-clients/google-cloud-spanner/src/test/java/com/google/cloud/spanner/SpannerOptionsTest.java b/google-cloud-clients/google-cloud-spanner/src/test/java/com/google/cloud/spanner/SpannerOptionsTest.java index a667b7385ae2..73155c84e4f2 100644 --- a/google-cloud-clients/google-cloud-spanner/src/test/java/com/google/cloud/spanner/SpannerOptionsTest.java +++ b/google-cloud-clients/google-cloud-spanner/src/test/java/com/google/cloud/spanner/SpannerOptionsTest.java @@ -368,14 +368,17 @@ public void testDoNotCacheClosedSpannerInstance() { Spanner service1 = options.getService(); Spanner service2 = options.getService(); assertThat(service1 == service2, is(true)); + assertThat(service1.isClosed()).isFalse(); // Closing a service instance should cause the SpannerOptions to create a new service. service1.close(); Spanner service3 = options.getService(); assertThat(service3 == service1, is(false)); - assertThat(service3.isClosed(), is(false)); + assertThat(service1.isClosed()).isTrue(); + assertThat(service3.isClosed()).isFalse();; // Getting another service from the SpannerOptions should return the new cached instance. Spanner service4 = options.getService(); assertThat(service3 == service4, is(true)); + assertThat(service3.isClosed()).isFalse(); service3.close(); } } From 1ca1c924bf46236fcac4c759e530d1368957aadb Mon Sep 17 00:00:00 2001 From: Olav Loite Date: Thu, 27 Jun 2019 13:42:02 +0200 Subject: [PATCH 11/11] fixed formatting --- .../test/java/com/google/cloud/spanner/SpannerOptionsTest.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/google-cloud-clients/google-cloud-spanner/src/test/java/com/google/cloud/spanner/SpannerOptionsTest.java b/google-cloud-clients/google-cloud-spanner/src/test/java/com/google/cloud/spanner/SpannerOptionsTest.java index 73155c84e4f2..3a85b2c916d0 100644 --- a/google-cloud-clients/google-cloud-spanner/src/test/java/com/google/cloud/spanner/SpannerOptionsTest.java +++ b/google-cloud-clients/google-cloud-spanner/src/test/java/com/google/cloud/spanner/SpannerOptionsTest.java @@ -374,7 +374,8 @@ public void testDoNotCacheClosedSpannerInstance() { Spanner service3 = options.getService(); assertThat(service3 == service1, is(false)); assertThat(service1.isClosed()).isTrue(); - assertThat(service3.isClosed()).isFalse();; + assertThat(service3.isClosed()).isFalse(); + ; // Getting another service from the SpannerOptions should return the new cached instance. Spanner service4 = options.getService(); assertThat(service3 == service4, is(true));