From 8a1d6902c3ea7b2751d70b0c83e6d0f6793b5451 Mon Sep 17 00:00:00 2001 From: Thiago Nunes Date: Tue, 21 Jul 2020 09:31:58 +1000 Subject: [PATCH 1/7] fix: retries PDML transactions on EOS errors It is possible to have the stream closed with an EOS internal error. This should be retried by the client. On this PR we add this retry logic. --- .../spanner/PartitionedDMLTransaction.java | 65 ++++++++------- .../PartitionedDmlTransactionTest.java | 81 ++++++++++++++++--- 2 files changed, 109 insertions(+), 37 deletions(-) diff --git a/google-cloud-spanner/src/main/java/com/google/cloud/spanner/PartitionedDMLTransaction.java b/google-cloud-spanner/src/main/java/com/google/cloud/spanner/PartitionedDMLTransaction.java index a938d174476..1c4acbad0b8 100644 --- a/google-cloud-spanner/src/main/java/com/google/cloud/spanner/PartitionedDMLTransaction.java +++ b/google-cloud-spanner/src/main/java/com/google/cloud/spanner/PartitionedDMLTransaction.java @@ -20,6 +20,7 @@ import com.google.api.gax.grpc.GrpcStatusCode; import com.google.api.gax.rpc.DeadlineExceededException; +import com.google.api.gax.rpc.InternalException; import com.google.api.gax.rpc.ServerStream; import com.google.api.gax.rpc.UnavailableException; import com.google.cloud.spanner.SessionImpl.SessionTransaction; @@ -55,23 +56,6 @@ class PartitionedDMLTransaction implements SessionTransaction { this.rpc = rpc; } - private ByteString initTransaction() { - final BeginTransactionRequest request = - BeginTransactionRequest.newBuilder() - .setSession(session.getName()) - .setOptions( - TransactionOptions.newBuilder() - .setPartitionedDml(TransactionOptions.PartitionedDml.getDefaultInstance())) - .build(); - Transaction txn = rpc.beginTransaction(request, session.getOptions()); - if (txn.getId().isEmpty()) { - throw SpannerExceptionFactory.newSpannerException( - ErrorCode.INTERNAL, - "Failed to init transaction, missing transaction id\n" + session.getName()); - } - return txn.getId(); - } - /** * Executes the {@link Statement} using a partitioned dml transaction with automatic retry if the * transaction was aborted. The update method uses the ExecuteStreamingSql RPC to execute the @@ -127,20 +111,24 @@ long executeStreamingPartitionedUpdate(final Statement statement, final Duration } } break; - } catch (UnavailableException e) { + } catch (UnavailableException | InternalException e) { // Retry the stream in the same transaction if the stream breaks with // UnavailableException and we have a resume token. Otherwise, we just retry the // entire transaction. - if (!ByteString.EMPTY.equals(resumeToken)) { - log.log( - Level.FINER, - "Retrying PartitionedDml stream using resume token '" - + resumeToken.toStringUtf8() - + "' because of broken stream", - e); + if (shouldResumeOrRestartTransaction(e)) { + if (!ByteString.EMPTY.equals(resumeToken)) { + log.log( + Level.FINER, + "Retrying PartitionedDml stream using resume token '" + + resumeToken.toStringUtf8() + + "' because of broken stream", + e); + } else { + throw new com.google.api.gax.rpc.AbortedException( + e, GrpcStatusCode.of(Code.ABORTED), true); + } } else { - throw new com.google.api.gax.rpc.AbortedException( - e, GrpcStatusCode.of(Code.ABORTED), true); + throw e; } } } @@ -174,4 +162,27 @@ public void invalidate() { // No-op method needed to implement SessionTransaction interface. @Override public void setSpan(Span span) {} + + private ByteString initTransaction() { + final BeginTransactionRequest request = + BeginTransactionRequest.newBuilder() + .setSession(session.getName()) + .setOptions( + TransactionOptions.newBuilder() + .setPartitionedDml(TransactionOptions.PartitionedDml.getDefaultInstance())) + .build(); + Transaction txn = rpc.beginTransaction(request, session.getOptions()); + if (txn.getId().isEmpty()) { + throw SpannerExceptionFactory.newSpannerException( + ErrorCode.INTERNAL, + "Failed to init transaction, missing transaction id\n" + session.getName()); + } + return txn.getId(); + } + + private boolean shouldResumeOrRestartTransaction(Exception e) { + return e instanceof UnavailableException + || (e instanceof InternalException + && e.getMessage().contains("Received unexpected EOS on DATA frame from server")); + } } diff --git a/google-cloud-spanner/src/test/java/com/google/cloud/spanner/PartitionedDmlTransactionTest.java b/google-cloud-spanner/src/test/java/com/google/cloud/spanner/PartitionedDmlTransactionTest.java index 8a158eb12e6..6b389970070 100644 --- a/google-cloud-spanner/src/test/java/com/google/cloud/spanner/PartitionedDmlTransactionTest.java +++ b/google-cloud-spanner/src/test/java/com/google/cloud/spanner/PartitionedDmlTransactionTest.java @@ -20,13 +20,11 @@ import static org.junit.Assert.fail; import static org.mockito.Matchers.any; import static org.mockito.Matchers.anyMap; -import static org.mockito.Mockito.mock; -import static org.mockito.Mockito.times; -import static org.mockito.Mockito.verify; -import static org.mockito.Mockito.when; +import static org.mockito.Mockito.*; import com.google.api.gax.grpc.GrpcStatusCode; import com.google.api.gax.rpc.AbortedException; +import com.google.api.gax.rpc.InternalException; import com.google.api.gax.rpc.ServerStream; import com.google.api.gax.rpc.UnavailableException; import com.google.cloud.spanner.spi.v1.SpannerRpc; @@ -34,13 +32,8 @@ import com.google.common.base.Ticker; import com.google.common.collect.ImmutableList; import com.google.protobuf.ByteString; -import com.google.spanner.v1.BeginTransactionRequest; -import com.google.spanner.v1.ExecuteSqlRequest; +import com.google.spanner.v1.*; import com.google.spanner.v1.ExecuteSqlRequest.QueryMode; -import com.google.spanner.v1.PartialResultSet; -import com.google.spanner.v1.ResultSetStats; -import com.google.spanner.v1.Transaction; -import com.google.spanner.v1.TransactionSelector; import io.grpc.Status.Code; import java.util.Collections; import java.util.Iterator; @@ -295,4 +288,72 @@ public Long answer(InvocationOnMock invocation) throws Throwable { Mockito.eq(executeRequestWithoutResumeToken), anyMap(), any(Duration.class)); } } + + @Test + public void testExecuteStreamingPartitionedUpdateUnexpectedEOS() { + ResultSetStats stats = ResultSetStats.newBuilder().setRowCountLowerBound(1000L).build(); + PartialResultSet p1 = PartialResultSet.newBuilder().setResumeToken(resumeToken).build(); + PartialResultSet p2 = PartialResultSet.newBuilder().setStats(stats).build(); + ServerStream stream1 = mock(ServerStream.class); + Iterator iterator = mock(Iterator.class); + when(iterator.hasNext()).thenReturn(true, true, false); + when(iterator.next()) + .thenReturn(p1) + .thenThrow( + new InternalException( + "INTERNAL: Received unexpected EOS on DATA frame from server.", + null, + GrpcStatusCode.of(Code.INTERNAL), + false)); + when(stream1.iterator()).thenReturn(iterator); + ServerStream stream2 = mock(ServerStream.class); + when(stream2.iterator()).thenReturn(ImmutableList.of(p1, p2).iterator()); + when(rpc.executeStreamingPartitionedDml( + Mockito.eq(executeRequestWithoutResumeToken), anyMap(), any(Duration.class))) + .thenReturn(stream1); + when(rpc.executeStreamingPartitionedDml( + Mockito.eq(executeRequestWithResumeToken), anyMap(), any(Duration.class))) + .thenReturn(stream2); + + PartitionedDMLTransaction tx = new PartitionedDMLTransaction(session, rpc); + long count = tx.executeStreamingPartitionedUpdate(Statement.of(sql), Duration.ofMinutes(10)); + + assertThat(count).isEqualTo(1000L); + verify(rpc).beginTransaction(any(BeginTransactionRequest.class), anyMap()); + verify(rpc) + .executeStreamingPartitionedDml( + Mockito.eq(executeRequestWithoutResumeToken), anyMap(), any(Duration.class)); + verify(rpc) + .executeStreamingPartitionedDml( + Mockito.eq(executeRequestWithResumeToken), anyMap(), any(Duration.class)); + } + + @Test + public void testExecuteStreamingPartitionedUpdateGenericInternalException() { + PartialResultSet p1 = PartialResultSet.newBuilder().setResumeToken(resumeToken).build(); + ServerStream stream1 = mock(ServerStream.class); + Iterator iterator = mock(Iterator.class); + when(iterator.hasNext()).thenReturn(true, true, false); + when(iterator.next()) + .thenReturn(p1) + .thenThrow( + new InternalException( + "INTERNAL: Error", null, GrpcStatusCode.of(Code.INTERNAL), false)); + when(stream1.iterator()).thenReturn(iterator); + when(rpc.executeStreamingPartitionedDml( + Mockito.eq(executeRequestWithoutResumeToken), anyMap(), any(Duration.class))) + .thenReturn(stream1); + + try { + PartitionedDMLTransaction tx = new PartitionedDMLTransaction(session, rpc); + tx.executeStreamingPartitionedUpdate(Statement.of(sql), Duration.ofMinutes(10)); + fail("missing expected INTERNAL exception"); + } catch (SpannerException e) { + assertThat(e.getErrorCode()).isEqualTo(ErrorCode.INTERNAL); + verify(rpc).beginTransaction(any(BeginTransactionRequest.class), anyMap()); + verify(rpc) + .executeStreamingPartitionedDml( + Mockito.eq(executeRequestWithoutResumeToken), anyMap(), any(Duration.class)); + } + } } From 88f4bb59c750b2c9ce2da5f7e881a1c4efcb98cc Mon Sep 17 00:00:00 2001 From: Thiago Nunes Date: Thu, 23 Jul 2020 12:03:20 +1000 Subject: [PATCH 2/7] fix: retries InternalException on PDML transaction When the exception is an EOS, we should retry the exception. --- .../cloud/spanner/PartitionedDmlTransaction.java | 10 +++++++++- .../cloud/spanner/PartitionedDmlTransactionTest.java | 6 +++--- 2 files changed, 12 insertions(+), 4 deletions(-) diff --git a/google-cloud-spanner/src/main/java/com/google/cloud/spanner/PartitionedDmlTransaction.java b/google-cloud-spanner/src/main/java/com/google/cloud/spanner/PartitionedDmlTransaction.java index 4d2b01146e8..8e9844b42e6 100644 --- a/google-cloud-spanner/src/main/java/com/google/cloud/spanner/PartitionedDmlTransaction.java +++ b/google-cloud-spanner/src/main/java/com/google/cloud/spanner/PartitionedDmlTransaction.java @@ -21,6 +21,7 @@ import com.google.api.gax.grpc.GrpcStatusCode; import com.google.api.gax.rpc.AbortedException; import com.google.api.gax.rpc.DeadlineExceededException; +import com.google.api.gax.rpc.InternalException; import com.google.api.gax.rpc.ServerStream; import com.google.api.gax.rpc.UnavailableException; import com.google.cloud.spanner.spi.v1.SpannerRpc; @@ -43,6 +44,7 @@ import org.threeten.bp.temporal.ChronoUnit; public class PartitionedDmlTransaction implements SessionImpl.SessionTransaction { + private static final Logger LOGGER = Logger.getLogger(PartitionedDmlTransaction.class.getName()); private final SessionImpl session; @@ -95,6 +97,11 @@ long executeStreamingPartitionedUpdate(final Statement statement, final Duration LOGGER.log( Level.FINER, "Retrying PartitionedDml transaction after UnavailableException", e); request = resumeOrRestartRequest(resumeToken, statement, request); + } catch (InternalException e) { + if (!e.getMessage().contains("Received unexpected EOS on DATA frame from server")) throw e; + LOGGER.log( + Level.FINER, "Retrying PartitionedDml transaction after InternalException - EOS", e); + request = resumeOrRestartRequest(resumeToken, statement, request); } catch (AbortedException e) { LOGGER.log(Level.FINER, "Retrying PartitionedDml transaction after AbortedException", e); resumeToken = ByteString.EMPTY; @@ -122,7 +129,8 @@ public void invalidate() { // No-op method needed to implement SessionTransaction interface. @Override - public void setSpan(Span span) {} + public void setSpan(Span span) { + } private Duration tryUpdateTimeout(final Duration timeout, final Stopwatch stopwatch) { final Duration remainingTimeout = diff --git a/google-cloud-spanner/src/test/java/com/google/cloud/spanner/PartitionedDmlTransactionTest.java b/google-cloud-spanner/src/test/java/com/google/cloud/spanner/PartitionedDmlTransactionTest.java index 355ef3f0a90..a38cd71baa1 100644 --- a/google-cloud-spanner/src/test/java/com/google/cloud/spanner/PartitionedDmlTransactionTest.java +++ b/google-cloud-spanner/src/test/java/com/google/cloud/spanner/PartitionedDmlTransactionTest.java @@ -283,7 +283,7 @@ public void testExecuteStreamingPartitionedUpdateUnexpectedEOS() { "INTERNAL: Received unexpected EOS on DATA frame from server.", null, GrpcStatusCode.of(Code.INTERNAL), - false)); + true)); when(stream1.iterator()).thenReturn(iterator); ServerStream stream2 = mock(ServerStream.class); when(stream2.iterator()).thenReturn(ImmutableList.of(p1, p2).iterator()); @@ -294,7 +294,7 @@ public void testExecuteStreamingPartitionedUpdateUnexpectedEOS() { Mockito.eq(executeRequestWithResumeToken), anyMap(), any(Duration.class))) .thenReturn(stream2); - PartitionedDMLTransaction tx = new PartitionedDMLTransaction(session, rpc); + PartitionedDmlTransaction tx = new PartitionedDmlTransaction(session, rpc, ticker); long count = tx.executeStreamingPartitionedUpdate(Statement.of(sql), Duration.ofMinutes(10)); assertThat(count).isEqualTo(1000L); @@ -324,7 +324,7 @@ public void testExecuteStreamingPartitionedUpdateGenericInternalException() { .thenReturn(stream1); try { - PartitionedDMLTransaction tx = new PartitionedDMLTransaction(session, rpc); + PartitionedDmlTransaction tx = new PartitionedDmlTransaction(session, rpc, ticker); tx.executeStreamingPartitionedUpdate(Statement.of(sql), Duration.ofMinutes(10)); fail("missing expected INTERNAL exception"); } catch (SpannerException e) { From b1450589f590dff2511ebe0d309e142019443fdb Mon Sep 17 00:00:00 2001 From: Thiago Nunes Date: Thu, 23 Jul 2020 16:54:57 +1000 Subject: [PATCH 3/7] refactor: re-uses spanner exception retry logic Re-uses the retry logic applied by the spanner exception factory for retrying EOS internal exceptions in pdml transactions. --- .../spanner/IsRetryableInternalError.java | 37 +++++ .../spanner/IsSslHandshakeException.java | 13 ++ .../spanner/PartitionedDmlTransaction.java | 7 +- .../spanner/SpannerExceptionFactory.java | 36 +---- .../spanner/IsRetryableInternalErrorTest.java | 128 ++++++++++++++++++ .../spanner/IsSslHandshakeExceptionTest.java | 42 ++++++ 6 files changed, 230 insertions(+), 33 deletions(-) create mode 100644 google-cloud-spanner/src/main/java/com/google/cloud/spanner/IsRetryableInternalError.java create mode 100644 google-cloud-spanner/src/main/java/com/google/cloud/spanner/IsSslHandshakeException.java create mode 100644 google-cloud-spanner/src/test/java/com/google/cloud/spanner/IsRetryableInternalErrorTest.java create mode 100644 google-cloud-spanner/src/test/java/com/google/cloud/spanner/IsSslHandshakeExceptionTest.java diff --git a/google-cloud-spanner/src/main/java/com/google/cloud/spanner/IsRetryableInternalError.java b/google-cloud-spanner/src/main/java/com/google/cloud/spanner/IsRetryableInternalError.java new file mode 100644 index 00000000000..5da79be665f --- /dev/null +++ b/google-cloud-spanner/src/main/java/com/google/cloud/spanner/IsRetryableInternalError.java @@ -0,0 +1,37 @@ +package com.google.cloud.spanner; + +import com.google.api.gax.rpc.InternalException; +import com.google.common.base.Predicate; +import io.grpc.Status; +import io.grpc.StatusRuntimeException; +import org.checkerframework.checker.nullness.compatqual.NullableDecl; + +public class IsRetryableInternalError implements Predicate { + + private static final String HTTP2_ERROR_MESSAGE = "HTTP/2 error code: INTERNAL_ERROR"; + private static final String CONNECTION_CLOSED_ERROR_MESSAGE = "Connection closed with unknown cause"; + private static final String EOS_ERROR_MESSAGE = "Received unexpected EOS on DATA frame from server"; + + @Override + public boolean apply(@NullableDecl Throwable cause) { + if (isInternalError(cause)) { + if (cause.getMessage().contains(HTTP2_ERROR_MESSAGE)) { + // See b/25451313. + return true; + } else if (cause.getMessage().contains(CONNECTION_CLOSED_ERROR_MESSAGE)) { + // See b/27794742. + return true; + } else if (cause.getMessage().contains(EOS_ERROR_MESSAGE)) { + return true; + } + } + return false; + } + + private boolean isInternalError(Throwable cause) { + return (cause instanceof InternalException) + || (cause instanceof StatusRuntimeException + && ((StatusRuntimeException) cause).getStatus().getCode() + == Status.Code.INTERNAL); + } +} diff --git a/google-cloud-spanner/src/main/java/com/google/cloud/spanner/IsSslHandshakeException.java b/google-cloud-spanner/src/main/java/com/google/cloud/spanner/IsSslHandshakeException.java new file mode 100644 index 00000000000..1ad78734cfd --- /dev/null +++ b/google-cloud-spanner/src/main/java/com/google/cloud/spanner/IsSslHandshakeException.java @@ -0,0 +1,13 @@ +package com.google.cloud.spanner; + +import com.google.common.base.Predicate; +import javax.net.ssl.SSLHandshakeException; +import org.checkerframework.checker.nullness.compatqual.NullableDecl; + +public class IsSslHandshakeException implements Predicate { + + @Override + public boolean apply(@NullableDecl Throwable input) { + return input instanceof SSLHandshakeException; + } +} diff --git a/google-cloud-spanner/src/main/java/com/google/cloud/spanner/PartitionedDmlTransaction.java b/google-cloud-spanner/src/main/java/com/google/cloud/spanner/PartitionedDmlTransaction.java index 8e9844b42e6..31dddddc844 100644 --- a/google-cloud-spanner/src/main/java/com/google/cloud/spanner/PartitionedDmlTransaction.java +++ b/google-cloud-spanner/src/main/java/com/google/cloud/spanner/PartitionedDmlTransaction.java @@ -50,12 +50,14 @@ public class PartitionedDmlTransaction implements SessionImpl.SessionTransaction private final SessionImpl session; private final SpannerRpc rpc; private final Ticker ticker; + private final IsRetryableInternalError isRetryableInternalErrorPredicate; private volatile boolean isValid = true; PartitionedDmlTransaction(SessionImpl session, SpannerRpc rpc, Ticker ticker) { this.session = session; this.rpc = rpc; this.ticker = ticker; + this.isRetryableInternalErrorPredicate = new IsRetryableInternalError(); } /** @@ -98,7 +100,10 @@ long executeStreamingPartitionedUpdate(final Statement statement, final Duration Level.FINER, "Retrying PartitionedDml transaction after UnavailableException", e); request = resumeOrRestartRequest(resumeToken, statement, request); } catch (InternalException e) { - if (!e.getMessage().contains("Received unexpected EOS on DATA frame from server")) throw e; + if (!isRetryableInternalErrorPredicate.apply(e)) { + throw e; + } + LOGGER.log( Level.FINER, "Retrying PartitionedDml transaction after InternalException - EOS", e); request = resumeOrRestartRequest(resumeToken, statement, request); diff --git a/google-cloud-spanner/src/main/java/com/google/cloud/spanner/SpannerExceptionFactory.java b/google-cloud-spanner/src/main/java/com/google/cloud/spanner/SpannerExceptionFactory.java index 78cd23d0641..3fa756875b9 100644 --- a/google-cloud-spanner/src/main/java/com/google/cloud/spanner/SpannerExceptionFactory.java +++ b/google-cloud-spanner/src/main/java/com/google/cloud/spanner/SpannerExceptionFactory.java @@ -26,12 +26,10 @@ import io.grpc.Context; import io.grpc.Metadata; import io.grpc.Status; -import io.grpc.StatusRuntimeException; import io.grpc.protobuf.ProtoUtils; import java.util.concurrent.CancellationException; import java.util.concurrent.TimeoutException; import javax.annotation.Nullable; -import javax.net.ssl.SSLHandshakeException; /** * A factory for creating instances of {@link SpannerException} and its subtypes. All creation of @@ -40,6 +38,7 @@ * ErrorCode#ABORTED} are always represented by {@link AbortedException}. */ public final class SpannerExceptionFactory { + static final String SESSION_RESOURCE_TYPE = "type.googleapis.com/google.spanner.v1.Session"; static final String DATABASE_RESOURCE_TYPE = "type.googleapis.com/google.spanner.admin.database.v1.Database"; @@ -257,35 +256,8 @@ private static boolean hasCauseMatching( } private static class Matchers { - static final Predicate isRetryableInternalError = - new Predicate() { - @Override - public boolean apply(Throwable cause) { - if (cause instanceof StatusRuntimeException - && ((StatusRuntimeException) cause).getStatus().getCode() == Status.Code.INTERNAL) { - if (cause.getMessage().contains("HTTP/2 error code: INTERNAL_ERROR")) { - // See b/25451313. - return true; - } - if (cause.getMessage().contains("Connection closed with unknown cause")) { - // See b/27794742. - return true; - } - if (cause - .getMessage() - .contains("Received unexpected EOS on DATA frame from server")) { - return true; - } - } - return false; - } - }; - static final Predicate isSSLHandshakeException = - new Predicate() { - @Override - public boolean apply(Throwable input) { - return input instanceof SSLHandshakeException; - } - }; + + static final Predicate isRetryableInternalError = new IsRetryableInternalError(); + static final Predicate isSSLHandshakeException = new IsSslHandshakeException(); } } diff --git a/google-cloud-spanner/src/test/java/com/google/cloud/spanner/IsRetryableInternalErrorTest.java b/google-cloud-spanner/src/test/java/com/google/cloud/spanner/IsRetryableInternalErrorTest.java new file mode 100644 index 00000000000..3795269acfe --- /dev/null +++ b/google-cloud-spanner/src/test/java/com/google/cloud/spanner/IsRetryableInternalErrorTest.java @@ -0,0 +1,128 @@ +package com.google.cloud.spanner; + +import static com.google.common.truth.Truth.assertThat; + +import com.google.api.gax.grpc.GrpcStatusCode; +import com.google.api.gax.rpc.InternalException; +import com.google.common.base.Predicate; +import io.grpc.Status; +import io.grpc.Status.Code; +import io.grpc.StatusRuntimeException; +import org.junit.Before; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.junit.runners.JUnit4; + +@SuppressWarnings("unchecked") +@RunWith(JUnit4.class) +public class IsRetryableInternalErrorTest { + + private Predicate predicate; + + @Before + public void setUp() { + predicate = new IsRetryableInternalError(); + } + + @Test + public void http2ErrorStatusRuntimeExceptionIsRetryable() { + final StatusRuntimeException e = new StatusRuntimeException( + Status + .fromCode(Code.INTERNAL) + .withDescription("INTERNAL: HTTP/2 error code: INTERNAL_ERROR.") + ); + + assertThat(predicate.apply(e)).isTrue(); + } + + @Test + public void http2ErrorInternalExceptionIsRetryable() { + final InternalException e = new InternalException( + "INTERNAL: HTTP/2 error code: INTERNAL_ERROR.", + null, + GrpcStatusCode.of(Code.INTERNAL), + false + ); + + assertThat(predicate.apply(e)).isTrue(); + } + + @Test + public void connectionClosedStatusRuntimeExceptionIsRetryable() { + final StatusRuntimeException e = new StatusRuntimeException( + Status + .fromCode(Code.INTERNAL) + .withDescription("INTERNAL: Connection closed with unknown cause.") + ); + + assertThat(predicate.apply(e)).isTrue(); + } + + @Test + public void connectionClosedInternalExceptionIsRetryable() { + final InternalException e = new InternalException( + "INTERNAL: Connection closed with unknown cause.", + null, + GrpcStatusCode.of(Code.INTERNAL), + false + ); + + assertThat(predicate.apply(e)).isTrue(); + } + + @Test + public void eosStatusRuntimeExceptionIsRetryable() { + final StatusRuntimeException e = new StatusRuntimeException( + Status + .fromCode(Code.INTERNAL) + .withDescription("INTERNAL: Received unexpected EOS on DATA frame from server.") + ); + + assertThat(predicate.apply(e)).isTrue(); + } + + @Test + public void eosInternalExceptionIsRetryable() { + final InternalException e = new InternalException( + "INTERNAL: Received unexpected EOS on DATA frame from server.", + null, + GrpcStatusCode.of(Code.INTERNAL), + false + ); + + assertThat(predicate.apply(e)).isTrue(); + } + + @Test + public void genericInternalStatusRuntimeExceptionIsRetryable() { + final StatusRuntimeException e = new StatusRuntimeException( + Status + .fromCode(Code.INTERNAL) + .withDescription("INTERNAL: Generic.") + ); + + assertThat(predicate.apply(e)).isFalse(); + } + + @Test + public void genericInternalExceptionIsNotRetryable() { + final InternalException e = new InternalException( + "INTERNAL: Generic.", + null, + GrpcStatusCode.of(Code.INTERNAL), + false + ); + + assertThat(predicate.apply(e)).isFalse(); + } + + @Test + public void nullIsNotRetryable() { + assertThat(predicate.apply(null)).isFalse(); + } + + @Test + public void genericExceptionIsNotRetryable() { + assertThat(predicate.apply(new Exception())).isFalse(); + } +} diff --git a/google-cloud-spanner/src/test/java/com/google/cloud/spanner/IsSslHandshakeExceptionTest.java b/google-cloud-spanner/src/test/java/com/google/cloud/spanner/IsSslHandshakeExceptionTest.java new file mode 100644 index 00000000000..50efa0338dc --- /dev/null +++ b/google-cloud-spanner/src/test/java/com/google/cloud/spanner/IsSslHandshakeExceptionTest.java @@ -0,0 +1,42 @@ +package com.google.cloud.spanner; + +import static com.google.common.truth.Truth.assertThat; + +import com.google.api.gax.grpc.GrpcStatusCode; +import com.google.api.gax.rpc.InternalException; +import com.google.common.base.Predicate; +import io.grpc.Status; +import io.grpc.Status.Code; +import io.grpc.StatusRuntimeException; +import javax.net.ssl.SSLHandshakeException; +import org.junit.Before; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.junit.runners.JUnit4; + +@SuppressWarnings("unchecked") +@RunWith(JUnit4.class) +public class IsSslHandshakeExceptionTest { + + private Predicate predicate; + + @Before + public void setUp() { + predicate = new IsSslHandshakeException(); + } + + @Test + public void sslHandshakeExceptionIsTrue() { + assertThat(predicate.apply(new SSLHandshakeException("test"))).isTrue(); + } + + @Test + public void genericExceptionIsNotSslHandshakeException() { + assertThat(predicate.apply(new Exception("test"))).isFalse(); + } + + @Test + public void nullIsNotSslHandshakeException() { + assertThat(predicate.apply(null)).isFalse(); + } +} From b8da6c1d96b04815a97024bc82d324a1b8abe221 Mon Sep 17 00:00:00 2001 From: Thiago Nunes Date: Thu, 23 Jul 2020 17:02:35 +1000 Subject: [PATCH 4/7] fix: adds / changes headers in files --- .../spanner/IsRetryableInternalError.java | 16 ++++++++++++++ .../spanner/IsSslHandshakeException.java | 16 ++++++++++++++ .../spanner/PartitionedDmlTransaction.java | 2 +- .../spanner/IsRetryableInternalErrorTest.java | 16 ++++++++++++++ .../spanner/IsSslHandshakeExceptionTest.java | 21 ++++++++++++++----- 5 files changed, 65 insertions(+), 6 deletions(-) diff --git a/google-cloud-spanner/src/main/java/com/google/cloud/spanner/IsRetryableInternalError.java b/google-cloud-spanner/src/main/java/com/google/cloud/spanner/IsRetryableInternalError.java index 5da79be665f..13915dc9ce1 100644 --- a/google-cloud-spanner/src/main/java/com/google/cloud/spanner/IsRetryableInternalError.java +++ b/google-cloud-spanner/src/main/java/com/google/cloud/spanner/IsRetryableInternalError.java @@ -1,3 +1,19 @@ +/* + * Copyright 2020 Google LLC + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + package com.google.cloud.spanner; import com.google.api.gax.rpc.InternalException; diff --git a/google-cloud-spanner/src/main/java/com/google/cloud/spanner/IsSslHandshakeException.java b/google-cloud-spanner/src/main/java/com/google/cloud/spanner/IsSslHandshakeException.java index 1ad78734cfd..cf582720c82 100644 --- a/google-cloud-spanner/src/main/java/com/google/cloud/spanner/IsSslHandshakeException.java +++ b/google-cloud-spanner/src/main/java/com/google/cloud/spanner/IsSslHandshakeException.java @@ -1,3 +1,19 @@ +/* + * Copyright 2020 Google LLC + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + package com.google.cloud.spanner; import com.google.common.base.Predicate; diff --git a/google-cloud-spanner/src/main/java/com/google/cloud/spanner/PartitionedDmlTransaction.java b/google-cloud-spanner/src/main/java/com/google/cloud/spanner/PartitionedDmlTransaction.java index 31dddddc844..2efb61748de 100644 --- a/google-cloud-spanner/src/main/java/com/google/cloud/spanner/PartitionedDmlTransaction.java +++ b/google-cloud-spanner/src/main/java/com/google/cloud/spanner/PartitionedDmlTransaction.java @@ -1,5 +1,5 @@ /* - * Copyright 2019 Google LLC + * Copyright 2020 Google LLC * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. diff --git a/google-cloud-spanner/src/test/java/com/google/cloud/spanner/IsRetryableInternalErrorTest.java b/google-cloud-spanner/src/test/java/com/google/cloud/spanner/IsRetryableInternalErrorTest.java index 3795269acfe..fe4d357dcd2 100644 --- a/google-cloud-spanner/src/test/java/com/google/cloud/spanner/IsRetryableInternalErrorTest.java +++ b/google-cloud-spanner/src/test/java/com/google/cloud/spanner/IsRetryableInternalErrorTest.java @@ -1,3 +1,19 @@ +/* + * Copyright 2020 Google LLC + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + package com.google.cloud.spanner; import static com.google.common.truth.Truth.assertThat; diff --git a/google-cloud-spanner/src/test/java/com/google/cloud/spanner/IsSslHandshakeExceptionTest.java b/google-cloud-spanner/src/test/java/com/google/cloud/spanner/IsSslHandshakeExceptionTest.java index 50efa0338dc..b73c2d6ef1b 100644 --- a/google-cloud-spanner/src/test/java/com/google/cloud/spanner/IsSslHandshakeExceptionTest.java +++ b/google-cloud-spanner/src/test/java/com/google/cloud/spanner/IsSslHandshakeExceptionTest.java @@ -1,13 +1,24 @@ +/* + * Copyright 2020 Google LLC + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + package com.google.cloud.spanner; import static com.google.common.truth.Truth.assertThat; -import com.google.api.gax.grpc.GrpcStatusCode; -import com.google.api.gax.rpc.InternalException; import com.google.common.base.Predicate; -import io.grpc.Status; -import io.grpc.Status.Code; -import io.grpc.StatusRuntimeException; import javax.net.ssl.SSLHandshakeException; import org.junit.Before; import org.junit.Test; From 3be1346fabeb75a83def2b23da95e8f4622bc0a6 Mon Sep 17 00:00:00 2001 From: Thiago Nunes Date: Thu, 23 Jul 2020 17:30:59 +1000 Subject: [PATCH 5/7] fix: fixes formatting --- .../spanner/IsRetryableInternalError.java | 9 ++- .../spanner/PartitionedDmlTransaction.java | 3 +- .../spanner/IsRetryableInternalErrorTest.java | 79 ++++++++----------- 3 files changed, 41 insertions(+), 50 deletions(-) diff --git a/google-cloud-spanner/src/main/java/com/google/cloud/spanner/IsRetryableInternalError.java b/google-cloud-spanner/src/main/java/com/google/cloud/spanner/IsRetryableInternalError.java index 13915dc9ce1..f3d17e3ce5a 100644 --- a/google-cloud-spanner/src/main/java/com/google/cloud/spanner/IsRetryableInternalError.java +++ b/google-cloud-spanner/src/main/java/com/google/cloud/spanner/IsRetryableInternalError.java @@ -25,8 +25,10 @@ public class IsRetryableInternalError implements Predicate { private static final String HTTP2_ERROR_MESSAGE = "HTTP/2 error code: INTERNAL_ERROR"; - private static final String CONNECTION_CLOSED_ERROR_MESSAGE = "Connection closed with unknown cause"; - private static final String EOS_ERROR_MESSAGE = "Received unexpected EOS on DATA frame from server"; + private static final String CONNECTION_CLOSED_ERROR_MESSAGE = + "Connection closed with unknown cause"; + private static final String EOS_ERROR_MESSAGE = + "Received unexpected EOS on DATA frame from server"; @Override public boolean apply(@NullableDecl Throwable cause) { @@ -47,7 +49,6 @@ public boolean apply(@NullableDecl Throwable cause) { private boolean isInternalError(Throwable cause) { return (cause instanceof InternalException) || (cause instanceof StatusRuntimeException - && ((StatusRuntimeException) cause).getStatus().getCode() - == Status.Code.INTERNAL); + && ((StatusRuntimeException) cause).getStatus().getCode() == Status.Code.INTERNAL); } } diff --git a/google-cloud-spanner/src/main/java/com/google/cloud/spanner/PartitionedDmlTransaction.java b/google-cloud-spanner/src/main/java/com/google/cloud/spanner/PartitionedDmlTransaction.java index 2efb61748de..da92b6f9c64 100644 --- a/google-cloud-spanner/src/main/java/com/google/cloud/spanner/PartitionedDmlTransaction.java +++ b/google-cloud-spanner/src/main/java/com/google/cloud/spanner/PartitionedDmlTransaction.java @@ -134,8 +134,7 @@ public void invalidate() { // No-op method needed to implement SessionTransaction interface. @Override - public void setSpan(Span span) { - } + public void setSpan(Span span) {} private Duration tryUpdateTimeout(final Duration timeout, final Stopwatch stopwatch) { final Duration remainingTimeout = diff --git a/google-cloud-spanner/src/test/java/com/google/cloud/spanner/IsRetryableInternalErrorTest.java b/google-cloud-spanner/src/test/java/com/google/cloud/spanner/IsRetryableInternalErrorTest.java index fe4d357dcd2..0f76da6692f 100644 --- a/google-cloud-spanner/src/test/java/com/google/cloud/spanner/IsRetryableInternalErrorTest.java +++ b/google-cloud-spanner/src/test/java/com/google/cloud/spanner/IsRetryableInternalErrorTest.java @@ -42,92 +42,83 @@ public void setUp() { @Test public void http2ErrorStatusRuntimeExceptionIsRetryable() { - final StatusRuntimeException e = new StatusRuntimeException( - Status - .fromCode(Code.INTERNAL) - .withDescription("INTERNAL: HTTP/2 error code: INTERNAL_ERROR.") - ); + final StatusRuntimeException e = + new StatusRuntimeException( + Status.fromCode(Code.INTERNAL) + .withDescription("INTERNAL: HTTP/2 error code: INTERNAL_ERROR.")); assertThat(predicate.apply(e)).isTrue(); } @Test public void http2ErrorInternalExceptionIsRetryable() { - final InternalException e = new InternalException( - "INTERNAL: HTTP/2 error code: INTERNAL_ERROR.", - null, - GrpcStatusCode.of(Code.INTERNAL), - false - ); + final InternalException e = + new InternalException( + "INTERNAL: HTTP/2 error code: INTERNAL_ERROR.", + null, + GrpcStatusCode.of(Code.INTERNAL), + false); assertThat(predicate.apply(e)).isTrue(); } @Test public void connectionClosedStatusRuntimeExceptionIsRetryable() { - final StatusRuntimeException e = new StatusRuntimeException( - Status - .fromCode(Code.INTERNAL) - .withDescription("INTERNAL: Connection closed with unknown cause.") - ); + final StatusRuntimeException e = + new StatusRuntimeException( + Status.fromCode(Code.INTERNAL) + .withDescription("INTERNAL: Connection closed with unknown cause.")); assertThat(predicate.apply(e)).isTrue(); } @Test public void connectionClosedInternalExceptionIsRetryable() { - final InternalException e = new InternalException( - "INTERNAL: Connection closed with unknown cause.", - null, - GrpcStatusCode.of(Code.INTERNAL), - false - ); + final InternalException e = + new InternalException( + "INTERNAL: Connection closed with unknown cause.", + null, + GrpcStatusCode.of(Code.INTERNAL), + false); assertThat(predicate.apply(e)).isTrue(); } @Test public void eosStatusRuntimeExceptionIsRetryable() { - final StatusRuntimeException e = new StatusRuntimeException( - Status - .fromCode(Code.INTERNAL) - .withDescription("INTERNAL: Received unexpected EOS on DATA frame from server.") - ); + final StatusRuntimeException e = + new StatusRuntimeException( + Status.fromCode(Code.INTERNAL) + .withDescription("INTERNAL: Received unexpected EOS on DATA frame from server.")); assertThat(predicate.apply(e)).isTrue(); } @Test public void eosInternalExceptionIsRetryable() { - final InternalException e = new InternalException( - "INTERNAL: Received unexpected EOS on DATA frame from server.", - null, - GrpcStatusCode.of(Code.INTERNAL), - false - ); + final InternalException e = + new InternalException( + "INTERNAL: Received unexpected EOS on DATA frame from server.", + null, + GrpcStatusCode.of(Code.INTERNAL), + false); assertThat(predicate.apply(e)).isTrue(); } @Test public void genericInternalStatusRuntimeExceptionIsRetryable() { - final StatusRuntimeException e = new StatusRuntimeException( - Status - .fromCode(Code.INTERNAL) - .withDescription("INTERNAL: Generic.") - ); + final StatusRuntimeException e = + new StatusRuntimeException( + Status.fromCode(Code.INTERNAL).withDescription("INTERNAL: Generic.")); assertThat(predicate.apply(e)).isFalse(); } @Test public void genericInternalExceptionIsNotRetryable() { - final InternalException e = new InternalException( - "INTERNAL: Generic.", - null, - GrpcStatusCode.of(Code.INTERNAL), - false - ); + final InternalException e = + new InternalException("INTERNAL: Generic.", null, GrpcStatusCode.of(Code.INTERNAL), false); assertThat(predicate.apply(e)).isFalse(); } From 5edd3eb3b6f42bf8c7a895a031359520da16b22d Mon Sep 17 00:00:00 2001 From: Thiago Nunes Date: Thu, 23 Jul 2020 17:33:43 +1000 Subject: [PATCH 6/7] refactor: removes unused annotation --- .../com/google/cloud/spanner/IsRetryableInternalError.java | 3 +-- .../java/com/google/cloud/spanner/IsSslHandshakeException.java | 3 +-- 2 files changed, 2 insertions(+), 4 deletions(-) diff --git a/google-cloud-spanner/src/main/java/com/google/cloud/spanner/IsRetryableInternalError.java b/google-cloud-spanner/src/main/java/com/google/cloud/spanner/IsRetryableInternalError.java index f3d17e3ce5a..cb048cb6735 100644 --- a/google-cloud-spanner/src/main/java/com/google/cloud/spanner/IsRetryableInternalError.java +++ b/google-cloud-spanner/src/main/java/com/google/cloud/spanner/IsRetryableInternalError.java @@ -20,7 +20,6 @@ import com.google.common.base.Predicate; import io.grpc.Status; import io.grpc.StatusRuntimeException; -import org.checkerframework.checker.nullness.compatqual.NullableDecl; public class IsRetryableInternalError implements Predicate { @@ -31,7 +30,7 @@ public class IsRetryableInternalError implements Predicate { "Received unexpected EOS on DATA frame from server"; @Override - public boolean apply(@NullableDecl Throwable cause) { + public boolean apply(Throwable cause) { if (isInternalError(cause)) { if (cause.getMessage().contains(HTTP2_ERROR_MESSAGE)) { // See b/25451313. diff --git a/google-cloud-spanner/src/main/java/com/google/cloud/spanner/IsSslHandshakeException.java b/google-cloud-spanner/src/main/java/com/google/cloud/spanner/IsSslHandshakeException.java index cf582720c82..53ff151a83c 100644 --- a/google-cloud-spanner/src/main/java/com/google/cloud/spanner/IsSslHandshakeException.java +++ b/google-cloud-spanner/src/main/java/com/google/cloud/spanner/IsSslHandshakeException.java @@ -18,12 +18,11 @@ import com.google.common.base.Predicate; import javax.net.ssl.SSLHandshakeException; -import org.checkerframework.checker.nullness.compatqual.NullableDecl; public class IsSslHandshakeException implements Predicate { @Override - public boolean apply(@NullableDecl Throwable input) { + public boolean apply(Throwable input) { return input instanceof SSLHandshakeException; } } From 1ff90921cfdffd2f59f6011deef97c60c23365a5 Mon Sep 17 00:00:00 2001 From: Thiago Nunes Date: Fri, 24 Jul 2020 09:24:53 +1000 Subject: [PATCH 7/7] dummy: to re-trigger tests