From 6cc7d886336be51ac2163b4f0d2f7221af012da9 Mon Sep 17 00:00:00 2001 From: Mairbek Khadikov Date: Mon, 20 Nov 2017 16:21:00 -0800 Subject: [PATCH 1/3] Treat RESOURCE_EXHAUSTED with retry information as aborted. --- .../cloud/spanner/AbortedException.java | 23 +---------------- .../cloud/spanner/SpannerException.java | 25 +++++++++++++++++++ .../spanner/SpannerExceptionFactory.java | 3 +++ .../com/google/cloud/spanner/SpannerImpl.java | 13 ++++------ .../spanner/TransactionRunnerImplTest.java | 22 +++++++++++++--- 5 files changed, 52 insertions(+), 34 deletions(-) diff --git a/google-cloud-spanner/src/main/java/com/google/cloud/spanner/AbortedException.java b/google-cloud-spanner/src/main/java/com/google/cloud/spanner/AbortedException.java index bc60203b22f8..874c4c63545e 100644 --- a/google-cloud-spanner/src/main/java/com/google/cloud/spanner/AbortedException.java +++ b/google-cloud-spanner/src/main/java/com/google/cloud/spanner/AbortedException.java @@ -16,11 +16,6 @@ package com.google.cloud.spanner; -import com.google.protobuf.util.Durations; -import com.google.rpc.RetryInfo; -import io.grpc.Metadata; -import io.grpc.Status; -import io.grpc.protobuf.ProtoUtils; import javax.annotation.Nullable; /** @@ -29,8 +24,6 @@ * other types of errors, most typically by retrying the transaction. */ public class AbortedException extends SpannerException { - private static final Metadata.Key KEY_RETRY_INFO = - ProtoUtils.keyForProto(RetryInfo.getDefaultInstance()); /** * Abort is not retryable per se: the operation request needs to change (generally to reflect a @@ -43,21 +36,7 @@ public class AbortedException extends SpannerException { DoNotConstructDirectly token, @Nullable String message, @Nullable Throwable cause) { super(token, ErrorCode.ABORTED, IS_RETRYABLE, message, cause); } - - /** - * Return the retry delay for transaction in milliseconds. Return -1 if this does not specify any - * retry delay. In that case, clients should fall back to a locally computed retry delay. - */ public long getRetryDelayInMillis() { - if (this.getCause() != null) { - Metadata trailers = Status.trailersFromThrowable(this.getCause()); - if (trailers != null && trailers.containsKey(KEY_RETRY_INFO)) { - RetryInfo retryInfo = trailers.get(KEY_RETRY_INFO); - if (retryInfo.hasRetryDelay()) { - return Durations.toMillis(retryInfo.getRetryDelay()); - } - } - } - return -1L; + return SpannerException.extractRetryDelay(getCause()); } } diff --git a/google-cloud-spanner/src/main/java/com/google/cloud/spanner/SpannerException.java b/google-cloud-spanner/src/main/java/com/google/cloud/spanner/SpannerException.java index 40de41cca7ab..062a81afbf4e 100644 --- a/google-cloud-spanner/src/main/java/com/google/cloud/spanner/SpannerException.java +++ b/google-cloud-spanner/src/main/java/com/google/cloud/spanner/SpannerException.java @@ -18,11 +18,18 @@ import com.google.cloud.grpc.BaseGrpcServiceException; import com.google.common.base.Preconditions; +import com.google.protobuf.util.Durations; +import com.google.rpc.RetryInfo; +import io.grpc.Metadata; +import io.grpc.Status; +import io.grpc.protobuf.ProtoUtils; import javax.annotation.Nullable; /** Base exception type for all exceptions produced by the Cloud Spanner service. */ public class SpannerException extends BaseGrpcServiceException { private static final long serialVersionUID = 20150916L; + private static final Metadata.Key KEY_RETRY_INFO = + ProtoUtils.keyForProto(RetryInfo.getDefaultInstance()); private final ErrorCode code; @@ -48,4 +55,22 @@ public ErrorCode getErrorCode() { enum DoNotConstructDirectly { ALLOWED } + + /** + * Return the retry delay for transaction in milliseconds. Return -1 if this does not specify any + * retry delay. In that case, clients should fall back to a locally computed retry delay. + */ + public static long extractRetryDelay(Throwable cause) { + if (cause != null) { + Metadata trailers = Status.trailersFromThrowable(cause); + if (trailers != null && trailers.containsKey(KEY_RETRY_INFO)) { + RetryInfo retryInfo = trailers.get(KEY_RETRY_INFO); + if (retryInfo.hasRetryDelay()) { + return Durations.toMillis(retryInfo.getRetryDelay()); + } + } + } + return -1L; + } + } 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 beb9ba2feb38..cddc030741a9 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 @@ -17,6 +17,7 @@ package com.google.cloud.spanner; import static com.google.cloud.spanner.SpannerException.DoNotConstructDirectly; +import static com.google.cloud.spanner.SpannerException.extractRetryDelay; import com.google.common.base.MoreObjects; import com.google.common.base.Predicate; @@ -126,6 +127,8 @@ private static boolean isRetryable(ErrorCode code, @Nullable Throwable cause) { return hasCauseMatching(cause, Matchers.isRetryableInternalError); case UNAVAILABLE: return true; + case RESOURCE_EXHAUSTED: + return extractRetryDelay(cause) > 0; default: return false; } diff --git a/google-cloud-spanner/src/main/java/com/google/cloud/spanner/SpannerImpl.java b/google-cloud-spanner/src/main/java/com/google/cloud/spanner/SpannerImpl.java index 2549747b9b1c..dbce0906aad3 100644 --- a/google-cloud-spanner/src/main/java/com/google/cloud/spanner/SpannerImpl.java +++ b/google-cloud-spanner/src/main/java/com/google/cloud/spanner/SpannerImpl.java @@ -1420,15 +1420,12 @@ TransactionSelector getTransactionSelector() { @Override public void onError(SpannerException e) { - if (e.getErrorCode() == ErrorCode.ABORTED) { - long delay = -1L; - if (e instanceof AbortedException) { - delay = ((AbortedException) e).getRetryDelayInMillis(); - } + long delay = SpannerException.extractRetryDelay(e.getCause()); + if (e.getErrorCode() == ErrorCode.ABORTED || (e.getErrorCode() == + ErrorCode.RESOURCE_EXHAUSTED && delay >= 0)) { if (delay == -1L) { txnLogger.log(Level.FINE, "Retry duration is missing from the exception.", e); } - synchronized (lock) { retryDelayInMillis = delay; aborted = true; @@ -2318,7 +2315,7 @@ abstract static class ResumableStreamIterator extends AbstractIterator backoffMillis = ArgumentCaptor.forClass(Long.class); verify(sleeper, times(1)).backoffSleep(Mockito.any(), backoffMillis.capture()); assertThat(backoffMillis.getValue()).isEqualTo(1001L); @@ -81,7 +81,7 @@ public void runAbortNoRetryInfo() { @Test public void commitAbort() { final SpannerException error = - SpannerExceptionFactory.newSpannerException(createRetryException()); + SpannerExceptionFactory.newSpannerException(createRetryException(Status.Code.ABORTED)); when(rpc.commit(Mockito.any(), Mockito.>any())) .thenThrow(error) .thenReturn( @@ -106,9 +106,23 @@ public Void run(TransactionContext transaction) throws Exception { assertThat(backoffMillis.getValue()).isEqualTo(1001L); } - private StatusRuntimeException createRetryException() { + @Test + public void runResourceExhausted() { + runTransaction(createRetryException(Status.Code.RESOURCE_EXHAUSTED)); + ArgumentCaptor backoffMillis = ArgumentCaptor.forClass(Long.class); + verify(sleeper, times(1)).backoffSleep(Mockito.any(), backoffMillis.capture()); + assertThat(backoffMillis.getValue()).isGreaterThan(0L); + } + + @Test(expected = SpannerException.class) + public void runResourceExhaustedNoRetry() throws Exception { + runTransaction( + new StatusRuntimeException(Status.fromCodeValue(Status.Code.RESOURCE_EXHAUSTED.value()))); + } + + private StatusRuntimeException createRetryException(Status.Code code) { Metadata.Key key = ProtoUtils.keyForProto(RetryInfo.getDefaultInstance()); - Status status = Status.fromCodeValue(Status.Code.ABORTED.value()); + Status status = Status.fromCodeValue(code.value()); Metadata trailers = new Metadata(); RetryInfo retryInfo = RetryInfo.newBuilder() From 2b8303d0106f803b6c81f8581c9e9101ec99b2bb Mon Sep 17 00:00:00 2001 From: Mairbek Khadikov Date: Tue, 28 Nov 2017 14:48:39 -0800 Subject: [PATCH 2/3] Updated according to comments --- .../cloud/spanner/AbortedException.java | 3 --- .../cloud/spanner/SpannerException.java | 8 ++++-- .../spanner/SpannerExceptionFactory.java | 3 +-- .../com/google/cloud/spanner/SpannerImpl.java | 27 ++++++++++++++----- .../spanner/SpannerExceptionFactoryTest.java | 27 +++++++++++++++++++ .../spanner/TransactionRunnerImplTest.java | 8 ------ 6 files changed, 54 insertions(+), 22 deletions(-) diff --git a/google-cloud-spanner/src/main/java/com/google/cloud/spanner/AbortedException.java b/google-cloud-spanner/src/main/java/com/google/cloud/spanner/AbortedException.java index 874c4c63545e..e8cdf53c7c54 100644 --- a/google-cloud-spanner/src/main/java/com/google/cloud/spanner/AbortedException.java +++ b/google-cloud-spanner/src/main/java/com/google/cloud/spanner/AbortedException.java @@ -36,7 +36,4 @@ public class AbortedException extends SpannerException { DoNotConstructDirectly token, @Nullable String message, @Nullable Throwable cause) { super(token, ErrorCode.ABORTED, IS_RETRYABLE, message, cause); } - public long getRetryDelayInMillis() { - return SpannerException.extractRetryDelay(getCause()); - } } diff --git a/google-cloud-spanner/src/main/java/com/google/cloud/spanner/SpannerException.java b/google-cloud-spanner/src/main/java/com/google/cloud/spanner/SpannerException.java index 062a81afbf4e..08b1ba0e8ef0 100644 --- a/google-cloud-spanner/src/main/java/com/google/cloud/spanner/SpannerException.java +++ b/google-cloud-spanner/src/main/java/com/google/cloud/spanner/SpannerException.java @@ -58,9 +58,13 @@ enum DoNotConstructDirectly { /** * Return the retry delay for transaction in milliseconds. Return -1 if this does not specify any - * retry delay. In that case, clients should fall back to a locally computed retry delay. + * retry delay. */ - public static long extractRetryDelay(Throwable cause) { + public long getRetryDelayInMillis() { + return extractRetryDelay(this.getCause()); + } + + static long extractRetryDelay(Throwable cause) { if (cause != null) { Metadata trailers = Status.trailersFromThrowable(cause); if (trailers != null && trailers.containsKey(KEY_RETRY_INFO)) { 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 cddc030741a9..a0a8008e8e8b 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 @@ -17,7 +17,6 @@ package com.google.cloud.spanner; import static com.google.cloud.spanner.SpannerException.DoNotConstructDirectly; -import static com.google.cloud.spanner.SpannerException.extractRetryDelay; import com.google.common.base.MoreObjects; import com.google.common.base.Predicate; @@ -128,7 +127,7 @@ private static boolean isRetryable(ErrorCode code, @Nullable Throwable cause) { case UNAVAILABLE: return true; case RESOURCE_EXHAUSTED: - return extractRetryDelay(cause) > 0; + return SpannerException.extractRetryDelay(cause) > 0; default: return false; } diff --git a/google-cloud-spanner/src/main/java/com/google/cloud/spanner/SpannerImpl.java b/google-cloud-spanner/src/main/java/com/google/cloud/spanner/SpannerImpl.java index dbce0906aad3..350755bcc5d2 100644 --- a/google-cloud-spanner/src/main/java/com/google/cloud/spanner/SpannerImpl.java +++ b/google-cloud-spanner/src/main/java/com/google/cloud/spanner/SpannerImpl.java @@ -231,7 +231,12 @@ static T runWithRetries(Callable callable) { throw e; } logger.log(Level.FINE, "Retryable exception, will sleep and retry", e); - backoffSleep(context, backOff); + long delay = e.getRetryDelayInMillis(); + if (delay != -1) { + backoffSleep(context, delay); + } else { + backoffSleep(context, backOff); + } } catch (Exception e) { Throwables.throwIfUnchecked(e); throw newSpannerException(ErrorCode.INTERNAL, "Unexpected exception thrown", e); @@ -1420,12 +1425,15 @@ TransactionSelector getTransactionSelector() { @Override public void onError(SpannerException e) { - long delay = SpannerException.extractRetryDelay(e.getCause()); - if (e.getErrorCode() == ErrorCode.ABORTED || (e.getErrorCode() == - ErrorCode.RESOURCE_EXHAUSTED && delay >= 0)) { + if (e.getErrorCode() == ErrorCode.ABORTED) { + long delay = -1L; + if (e instanceof AbortedException) { + delay = ((AbortedException) e).getRetryDelayInMillis(); + } if (delay == -1L) { txnLogger.log(Level.FINE, "Retry duration is missing from the exception.", e); } + synchronized (lock) { retryDelayInMillis = delay; aborted = true; @@ -2315,7 +2323,7 @@ abstract static class ResumableStreamIterator extends AbstractIterator key = ProtoUtils.keyForProto(RetryInfo.getDefaultInstance()); + RetryInfo retryInfo = + RetryInfo.newBuilder() + .setRetryDelay(Duration.newBuilder().setNanos(1000000).setSeconds(1L)) + .build(); + trailers.put(key, retryInfo); + SpannerException e = + SpannerExceptionFactory.newSpannerException(new StatusRuntimeException(status, trailers)); + assertThat(e.isRetryable()).isTrue(); + } + @Test public void abortWithRetryInfo() { Metadata.Key key = ProtoUtils.keyForProto(RetryInfo.getDefaultInstance()); diff --git a/google-cloud-spanner/src/test/java/com/google/cloud/spanner/TransactionRunnerImplTest.java b/google-cloud-spanner/src/test/java/com/google/cloud/spanner/TransactionRunnerImplTest.java index c803823bf1f5..e5f875a6e14b 100644 --- a/google-cloud-spanner/src/test/java/com/google/cloud/spanner/TransactionRunnerImplTest.java +++ b/google-cloud-spanner/src/test/java/com/google/cloud/spanner/TransactionRunnerImplTest.java @@ -106,14 +106,6 @@ public Void run(TransactionContext transaction) throws Exception { assertThat(backoffMillis.getValue()).isEqualTo(1001L); } - @Test - public void runResourceExhausted() { - runTransaction(createRetryException(Status.Code.RESOURCE_EXHAUSTED)); - ArgumentCaptor backoffMillis = ArgumentCaptor.forClass(Long.class); - verify(sleeper, times(1)).backoffSleep(Mockito.any(), backoffMillis.capture()); - assertThat(backoffMillis.getValue()).isGreaterThan(0L); - } - @Test(expected = SpannerException.class) public void runResourceExhaustedNoRetry() throws Exception { runTransaction( From 266054beb023bfedf8fddad39950fdd1174755aa Mon Sep 17 00:00:00 2001 From: Mairbek Khadikov Date: Wed, 20 Dec 2017 14:51:46 -0800 Subject: [PATCH 3/3] Updated according to comments --- .../main/java/com/google/cloud/spanner/SpannerException.java | 2 +- .../com/google/cloud/spanner/SpannerExceptionFactoryTest.java | 1 + 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/google-cloud-spanner/src/main/java/com/google/cloud/spanner/SpannerException.java b/google-cloud-spanner/src/main/java/com/google/cloud/spanner/SpannerException.java index 08b1ba0e8ef0..4c1203635621 100644 --- a/google-cloud-spanner/src/main/java/com/google/cloud/spanner/SpannerException.java +++ b/google-cloud-spanner/src/main/java/com/google/cloud/spanner/SpannerException.java @@ -57,7 +57,7 @@ enum DoNotConstructDirectly { } /** - * Return the retry delay for transaction in milliseconds. Return -1 if this does not specify any + * Return the retry delay for operation in milliseconds. Return -1 if this does not specify any * retry delay. */ public long getRetryDelayInMillis() { diff --git a/google-cloud-spanner/src/test/java/com/google/cloud/spanner/SpannerExceptionFactoryTest.java b/google-cloud-spanner/src/test/java/com/google/cloud/spanner/SpannerExceptionFactoryTest.java index 29e8358ff22a..d6fbf226ca26 100644 --- a/google-cloud-spanner/src/test/java/com/google/cloud/spanner/SpannerExceptionFactoryTest.java +++ b/google-cloud-spanner/src/test/java/com/google/cloud/spanner/SpannerExceptionFactoryTest.java @@ -76,6 +76,7 @@ public void resourceExhaustedWithBackoff() { SpannerException e = SpannerExceptionFactory.newSpannerException(new StatusRuntimeException(status, trailers)); assertThat(e.isRetryable()).isTrue(); + assertThat(e.getRetryDelayInMillis()).isEqualTo(1001); } @Test