Added ability to specify timeout unit in RequestRetryOptions (#17628)

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;
@@ -288,9 +287,9 @@ public Mono<HttpResponse> 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 +361,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 +407,9 @@ private Mono<HttpResponse> testDelayBounds(int primaryTryNumber, boolean tryingP
         private Mono<HttpResponse> testMaxDelayBounds(Mono<HttpResponse> 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");
                 }
 

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

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,18 +17,17 @@ 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;
 
     /**
      * 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.
      *
      * <p>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.</p>
@@ -55,8 +55,40 @@ 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, tryTimeoutInSeconds == null ? null : Duration.ofSeconds(tryTimeoutInSeconds),
+            retryDelayInMs == null ? null : Duration.ofMillis(retryDelayInMs),
+            maxRetryDelayInMs == null ? null : 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}.
+     *
+     * <p>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.</p>
+     * @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.
+     *
+     * <p>Before setting this understand the issues around reading stale and potentially-inconsistent data, view these
+     * <a href=https://docs.microsoft.com/en-us/azure/storage/common/storage-designing-ha-apps-with-ragrs>Azure Docs</a>
+     * for more information.</p>
+     * @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,40 +98,43 @@ 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 {
             /*
             Because this timeout applies to the whole operation, and calculating a meaningful timeout for read/write
             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 +148,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 +172,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 +212,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) {

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;