From df01db10a65a01159dcbdfbb3d26707283625477 Mon Sep 17 00:00:00 2001 From: gapra Date: Tue, 17 Nov 2020 12:03:24 -0800 Subject: [PATCH 1/3] Added ability to specify timeout unit in RequestRetryOptions --- .../storage/blob/RequestRetryTestFactory.java | 12 +- sdk/storage/azure-storage-common/CHANGELOG.md | 2 +- .../common/policy/RequestRetryOptions.java | 113 ++++++++++++++---- .../common/policy/RequestRetryPolicy.java | 2 +- 4 files changed, 95 insertions(+), 34 deletions(-) diff --git a/sdk/storage/azure-storage-blob/src/test/java/com/azure/storage/blob/RequestRetryTestFactory.java b/sdk/storage/azure-storage-blob/src/test/java/com/azure/storage/blob/RequestRetryTestFactory.java index a023d3cbdb41f..7ba1de1a2ae84 100644 --- a/sdk/storage/azure-storage-blob/src/test/java/com/azure/storage/blob/RequestRetryTestFactory.java +++ b/sdk/storage/azure-storage-blob/src/test/java/com/azure/storage/blob/RequestRetryTestFactory.java @@ -288,9 +288,9 @@ public Mono send(HttpRequest request) { switch (this.factory.tryNumber) { case 1: case 2: - return RETRY_TEST_OK_RESPONSE.delaySubscription(Duration.ofSeconds(options.getTryTimeout() + 1)); + return RETRY_TEST_OK_RESPONSE.delaySubscription(options.getTryTimeoutDuration().plusSeconds(1)); case 3: - return RETRY_TEST_OK_RESPONSE.delaySubscription(Duration.ofSeconds(options.getTryTimeout() - 1)); + return RETRY_TEST_OK_RESPONSE.delaySubscription(options.getTryTimeoutDuration().minusSeconds(1)); default: throw new IllegalArgumentException("Continued retrying after success"); } @@ -362,9 +362,9 @@ private long calcPrimaryDelay(int tryNumber) { switch (this.factory.retryTestScenario) { case RETRY_TEST_SCENARIO_EXPONENTIAL_TIMING: return (long) Math.ceil( - ((pow(2L, tryNumber - 1) - 1L) * this.factory.options.getRetryDelayInMs()) / 1000); + ((pow(2L, tryNumber - 1) - 1L) * this.factory.options.getRetryDelay().toMillis()) / 1000); case RETRY_TEST_SCENARIO_FIXED_TIMING: - return (long) Math.ceil(this.factory.options.getRetryDelayInMs() / 1000); + return (long) Math.ceil(this.factory.options.getRetryDelay().toMillis() / 1000); default: throw new IllegalArgumentException("Invalid test scenario"); } @@ -408,9 +408,9 @@ private Mono testDelayBounds(int primaryTryNumber, boolean tryingP private Mono testMaxDelayBounds(Mono response) { return Mono.defer(() -> Mono.fromCallable(() -> { OffsetDateTime now = OffsetDateTime.now(); - if (now.isAfter(factory.time.plusSeconds((long) Math.ceil((factory.options.getMaxRetryDelayInMs() / 1000) + 1)))) { + if (now.isAfter(factory.time.plusSeconds((long) Math.ceil((factory.options.getMaxRetryDelay().toMillis() / 1000) + 1)))) { throw new IllegalArgumentException("Max retry delay exceeded"); - } else if (now.isBefore(factory.time.plusSeconds((long) Math.ceil((factory.options.getMaxRetryDelayInMs() / 1000) - 1)))) { + } else if (now.isBefore(factory.time.plusSeconds((long) Math.ceil((factory.options.getMaxRetryDelay().toMillis() / 1000) - 1)))) { throw new IllegalArgumentException("Retry did not delay long enough"); } diff --git a/sdk/storage/azure-storage-common/CHANGELOG.md b/sdk/storage/azure-storage-common/CHANGELOG.md index 910ed0e37cf65..73074886b777f 100644 --- a/sdk/storage/azure-storage-common/CHANGELOG.md +++ b/sdk/storage/azure-storage-common/CHANGELOG.md @@ -1,7 +1,7 @@ # Release History ## 12.10.0-beta.1 (Unreleased) - +- Added ability to specify timeout units in RequestRetryOptions. ## 12.9.0 (2020-11-11) - GA release diff --git a/sdk/storage/azure-storage-common/src/main/java/com/azure/storage/common/policy/RequestRetryOptions.java b/sdk/storage/azure-storage-common/src/main/java/com/azure/storage/common/policy/RequestRetryOptions.java index e7e719f4c4a8b..c24666a42483d 100644 --- a/sdk/storage/azure-storage-common/src/main/java/com/azure/storage/common/policy/RequestRetryOptions.java +++ b/sdk/storage/azure-storage-common/src/main/java/com/azure/storage/common/policy/RequestRetryOptions.java @@ -7,7 +7,8 @@ import com.azure.core.util.logging.ClientLogger; import com.azure.storage.common.implementation.StorageImplUtils; -import java.util.concurrent.TimeUnit; + +import java.time.Duration; /** * Configuration options for {@link RequestRetryPolicy}. @@ -16,9 +17,9 @@ public final class RequestRetryOptions { private final ClientLogger logger = new ClientLogger(RequestRetryOptions.class); private final int maxTries; - private final int tryTimeout; - private final long retryDelayInMs; - private final long maxRetryDelayInMs; + private final Duration tryTimeout; + private final Duration retryDelay; + private final Duration maxRetryDelay; private final RetryPolicyType retryPolicyType; private final String secondaryHost; @@ -26,8 +27,7 @@ public final class RequestRetryOptions { * Configures how the {@link HttpPipeline} should retry requests. */ public RequestRetryOptions() { - this(RetryPolicyType.EXPONENTIAL, null, - null, null, null, null); + this(RetryPolicyType.EXPONENTIAL, null, (Integer) null, null, null, null); } /** @@ -36,8 +36,8 @@ public RequestRetryOptions() { * @param retryPolicyType Optional. A {@link RetryPolicyType} specifying the type of retry pattern to use, default * value is {@link RetryPolicyType#EXPONENTIAL EXPONENTIAL}. * @param maxTries Optional. Maximum number of attempts an operation will be retried, default is {@code 4}. - * @param tryTimeout Optional. Specified the maximum time allowed before a request is cancelled and assumed failed, - * default is {@link Integer#MAX_VALUE}. + * @param tryTimeoutInSeconds Optional. Specified the maximum time allowed before a request is cancelled and + * assumed failed, default is {@link Integer#MAX_VALUE} s. * *

This value should be based on the bandwidth available to the host machine and proximity to the Storage * service, a good starting point may be 60 seconds per MB of anticipated payload size.

@@ -55,8 +55,39 @@ public RequestRetryOptions() { * or non-null or {@code retryPolicyType} isn't {@link RetryPolicyType#EXPONENTIAL} * or {@link RetryPolicyType#FIXED}. */ - public RequestRetryOptions(RetryPolicyType retryPolicyType, Integer maxTries, Integer tryTimeout, + public RequestRetryOptions(RetryPolicyType retryPolicyType, Integer maxTries, Integer tryTimeoutInSeconds, Long retryDelayInMs, Long maxRetryDelayInMs, String secondaryHost) { + this(retryPolicyType, maxTries, Duration.ofSeconds(tryTimeoutInSeconds), Duration.ofMillis(retryDelayInMs), + Duration.ofMillis(maxRetryDelayInMs), secondaryHost); + } + + /** + * Configures how the {@link HttpPipeline} should retry requests. + * + * @param retryPolicyType Optional. A {@link RetryPolicyType} specifying the type of retry pattern to use, default + * value is {@link RetryPolicyType#EXPONENTIAL EXPONENTIAL}. + * @param maxTries Optional. Maximum number of attempts an operation will be retried, default is {@code 4}. + * @param tryTimeout Optional. Specified the maximum time allowed before a request is cancelled and + * assumed failed, default is {@link Integer#MAX_VALUE}. + * + *

This value should be based on the bandwidth available to the host machine and proximity to the Storage + * service, a good starting point may be 60 seconds per MB of anticipated payload size.

+ * @param retryDelay Optional. Specifies the amount of delay to use before retrying an operation, default value + * is {@code 4ms} when {@code retryPolicyType} is {@link RetryPolicyType#EXPONENTIAL EXPONENTIAL} and {@code 30ms} + * when {@code retryPolicyType} is {@link RetryPolicyType#FIXED FIXED}. + * @param maxRetryDelay Optional. Specifies the maximum delay allowed before retrying an operation, default + * value is {@code 120ms}. + * @param secondaryHost Optional. Specified a secondary Storage account to retry requests against, default is none. + * + *

Before setting this understand the issues around reading stale and potentially-inconsistent data, view these + * Azure Docs + * for more information.

+ * @throws IllegalArgumentException If {@code getRetryDelayInMs} and {@code getMaxRetryDelayInMs} are not both null + * or non-null or {@code retryPolicyType} isn't {@link RetryPolicyType#EXPONENTIAL} + * or {@link RetryPolicyType#FIXED}. + */ + public RequestRetryOptions(RetryPolicyType retryPolicyType, Integer maxTries, Duration tryTimeout, + Duration retryDelay, Duration maxRetryDelay, String secondaryHost) { this.retryPolicyType = retryPolicyType == null ? RetryPolicyType.EXPONENTIAL : retryPolicyType; if (maxTries != null) { StorageImplUtils.assertInBounds("maxRetries", maxTries, 1, Integer.MAX_VALUE); @@ -66,7 +97,8 @@ public RequestRetryOptions(RetryPolicyType retryPolicyType, Integer maxTries, In } if (tryTimeout != null) { - StorageImplUtils.assertInBounds("tryTimeout", tryTimeout, 1, Integer.MAX_VALUE); + StorageImplUtils.assertInBounds("'tryTimeout' in seconds", tryTimeout.getSeconds(), 1, + Integer.MAX_VALUE); this.tryTimeout = tryTimeout; } else { /* @@ -74,32 +106,34 @@ public RequestRetryOptions(RetryPolicyType retryPolicyType, Integer maxTries, In operations must consider the size of the payload, we can't set a meaningful default value for all requests and therefore default to no timeout. */ - this.tryTimeout = Integer.MAX_VALUE; + this.tryTimeout = Duration.ofSeconds(Integer.MAX_VALUE); } - if ((retryDelayInMs == null && maxRetryDelayInMs != null) - || (retryDelayInMs != null && maxRetryDelayInMs == null)) { + if ((retryDelay == null && maxRetryDelay != null) + || (retryDelay != null && maxRetryDelay == null)) { throw logger.logExceptionAsError( new IllegalArgumentException("Both retryDelay and maxRetryDelay must be null or neither can be null")); } - if (retryDelayInMs != null) { - StorageImplUtils.assertInBounds("maxRetryDelayInMs", maxRetryDelayInMs, 1, Long.MAX_VALUE); - StorageImplUtils.assertInBounds("retryDelayInMs", retryDelayInMs, 1, maxRetryDelayInMs); - this.maxRetryDelayInMs = maxRetryDelayInMs; - this.retryDelayInMs = retryDelayInMs; + if (retryDelay != null) { + StorageImplUtils.assertInBounds("'maxRetryDelay' in milliseconds", maxRetryDelay.toMillis(), 1, + Long.MAX_VALUE); + StorageImplUtils.assertInBounds("'retryDelay' in milliseconds", retryDelay.toMillis(), 1, + maxRetryDelay.toMillis()); + this.maxRetryDelay = maxRetryDelay; + this.retryDelay = retryDelay; } else { switch (this.retryPolicyType) { case EXPONENTIAL: - this.retryDelayInMs = TimeUnit.SECONDS.toMillis(4); + this.retryDelay = Duration.ofSeconds(4); break; case FIXED: - this.retryDelayInMs = TimeUnit.SECONDS.toMillis(30); + this.retryDelay = Duration.ofSeconds(30); break; default: throw logger.logExceptionAsError(new IllegalArgumentException("Invalid 'RetryPolicyType'.")); } - this.maxRetryDelayInMs = TimeUnit.SECONDS.toMillis(120); + this.maxRetryDelay = Duration.ofSeconds(120); } this.secondaryHost = secondaryHost; } @@ -113,8 +147,17 @@ public int getMaxTries() { /** * @return the maximum time, in seconds, allowed for a request until it is considered timed out. + * @deprecated Please use {@link RequestRetryOptions#getTryTimeoutDuration()} */ + @Deprecated public int getTryTimeout() { + return (int) this.tryTimeout.getSeconds(); + } + + /** + * @return the maximum time, in seconds, allowed for a request until it is considered timed out. + */ + public Duration getTryTimeoutDuration() { return this.tryTimeout; } @@ -128,16 +171,34 @@ public String getSecondaryHost() { /** * @return the delay in milliseconds between each retry attempt. + * @deprecated Please use {@link RequestRetryOptions#getTryTimeoutDuration()} */ + @Deprecated public long getRetryDelayInMs() { - return retryDelayInMs; + return retryDelay.toMillis(); + } + + /** + * @return the delay between each retry attempt. + */ + public Duration getRetryDelay() { + return retryDelay; } /** * @return the maximum delay in milliseconds allowed between each retry. + * @deprecated Please use {@link RequestRetryOptions#getTryTimeoutDuration()} */ + @Deprecated public long getMaxRetryDelayInMs() { - return maxRetryDelayInMs; + return maxRetryDelay.toMillis(); + } + + /** + * @return the maximum delay allowed between each retry. + */ + public Duration getMaxRetryDelay() { + return maxRetryDelay; } /** @@ -150,18 +211,18 @@ long calculateDelayInMs(int tryCount) { long delay; switch (this.retryPolicyType) { case EXPONENTIAL: - delay = (powOfTwo(tryCount - 1) - 1L) * this.retryDelayInMs; + delay = (powOfTwo(tryCount - 1) - 1L) * this.retryDelay.toMillis(); break; case FIXED: // The first try should have zero delay. Every other try has the fixed value - delay = tryCount > 1 ? this.retryDelayInMs : 0; + delay = tryCount > 1 ? this.retryDelay.toMillis() : 0; break; default: throw logger.logExceptionAsError(new IllegalArgumentException("Invalid retry policy type.")); } - return Math.min(delay, this.maxRetryDelayInMs); + return Math.min(delay, this.maxRetryDelay.toMillis()); } private long powOfTwo(int exponent) { diff --git a/sdk/storage/azure-storage-common/src/main/java/com/azure/storage/common/policy/RequestRetryPolicy.java b/sdk/storage/azure-storage-common/src/main/java/com/azure/storage/common/policy/RequestRetryPolicy.java index eee9c15f531dd..f7f4c08565f13 100644 --- a/sdk/storage/azure-storage-common/src/main/java/com/azure/storage/common/policy/RequestRetryPolicy.java +++ b/sdk/storage/azure-storage-common/src/main/java/com/azure/storage/common/policy/RequestRetryPolicy.java @@ -111,7 +111,7 @@ stream, the buffers that were emitted will have already been consumed (their pos until after the retry backoff delay, so we call delaySubscription. */ return next.clone().process() - .timeout(Duration.ofSeconds(this.requestRetryOptions.getTryTimeout())) + .timeout(this.requestRetryOptions.getTryTimeoutDuration()) .delaySubscription(Duration.ofMillis(delayMs)) .flatMap(response -> { boolean newConsiderSecondary = considerSecondary; From 5fa0e03d2c70ac64163911b483f510c5a4aa169d Mon Sep 17 00:00:00 2001 From: gapra Date: Tue, 17 Nov 2020 12:40:08 -0800 Subject: [PATCH 2/3] NPE fix --- .../com/azure/storage/common/policy/RequestRetryOptions.java | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/sdk/storage/azure-storage-common/src/main/java/com/azure/storage/common/policy/RequestRetryOptions.java b/sdk/storage/azure-storage-common/src/main/java/com/azure/storage/common/policy/RequestRetryOptions.java index c24666a42483d..36b3ddbde658f 100644 --- a/sdk/storage/azure-storage-common/src/main/java/com/azure/storage/common/policy/RequestRetryOptions.java +++ b/sdk/storage/azure-storage-common/src/main/java/com/azure/storage/common/policy/RequestRetryOptions.java @@ -57,8 +57,9 @@ public RequestRetryOptions() { */ public RequestRetryOptions(RetryPolicyType retryPolicyType, Integer maxTries, Integer tryTimeoutInSeconds, Long retryDelayInMs, Long maxRetryDelayInMs, String secondaryHost) { - this(retryPolicyType, maxTries, Duration.ofSeconds(tryTimeoutInSeconds), Duration.ofMillis(retryDelayInMs), - Duration.ofMillis(maxRetryDelayInMs), secondaryHost); + this(retryPolicyType, maxTries, tryTimeoutInSeconds == null ? null : Duration.ofSeconds(tryTimeoutInSeconds), + retryDelayInMs == null ? null : Duration.ofMillis(retryDelayInMs), + maxRetryDelayInMs == null ? null : Duration.ofMillis(maxRetryDelayInMs), secondaryHost); } /** From 4a87e24370d0115fb1af26150fc87aa3375697aa Mon Sep 17 00:00:00 2001 From: gapra Date: Tue, 17 Nov 2020 13:10:44 -0800 Subject: [PATCH 3/3] Analyze stuff --- .../java/com/azure/storage/blob/RequestRetryTestFactory.java | 1 - 1 file changed, 1 deletion(-) diff --git a/sdk/storage/azure-storage-blob/src/test/java/com/azure/storage/blob/RequestRetryTestFactory.java b/sdk/storage/azure-storage-blob/src/test/java/com/azure/storage/blob/RequestRetryTestFactory.java index 7ba1de1a2ae84..d6b2513dc3195 100644 --- a/sdk/storage/azure-storage-blob/src/test/java/com/azure/storage/blob/RequestRetryTestFactory.java +++ b/sdk/storage/azure-storage-blob/src/test/java/com/azure/storage/blob/RequestRetryTestFactory.java @@ -24,7 +24,6 @@ import java.net.URL; import java.nio.ByteBuffer; import java.nio.charset.Charset; -import java.time.Duration; import java.time.OffsetDateTime; import java.time.temporal.ChronoUnit; import java.util.concurrent.TimeoutException;