smiroslav commented on code in PR #1748:
URL: https://github.com/apache/jackrabbit-oak/pull/1748#discussion_r1840670006
##########
oak-segment-azure/src/main/java/org/apache/jackrabbit/oak/segment/azure/util/AzureRequestOptions.java:
##########
@@ -45,52 +43,43 @@ public class AzureRequestOptions {
private AzureRequestOptions() {
}
- /**
- * Apply default request options to the blobRequestOptions if they are not
already set.
- * @param blobRequestOptions
- */
- public static void applyDefaultRequestOptions(BlobRequestOptions
blobRequestOptions) {
- if (blobRequestOptions.getRetryPolicyFactory() == null) {
- int retryAttempts = Integer.getInteger(RETRY_ATTEMPTS_PROP,
DEFAULT_RETRY_ATTEMPTS);
- if (retryAttempts > 0) {
- Integer retryBackoffSeconds =
Integer.getInteger(RETRY_BACKOFF_PROP, DEFAULT_RETRY_BACKOFF_SECONDS);
- blobRequestOptions.setRetryPolicyFactory(new
RetryLinearRetry((int) TimeUnit.SECONDS.toMillis(retryBackoffSeconds),
retryAttempts));
- }
- }
- if (blobRequestOptions.getMaximumExecutionTimeInMs() == null) {
- int timeoutExecution = Integer.getInteger(TIMEOUT_EXECUTION_PROP,
DEFAULT_TIMEOUT_EXECUTION);
- if (timeoutExecution > 0) {
- blobRequestOptions.setMaximumExecutionTimeInMs((int)
TimeUnit.SECONDS.toMillis(timeoutExecution));
- }
- }
- if (blobRequestOptions.getTimeoutIntervalInMs() == null) {
- int timeoutInterval = Integer.getInteger(TIMEOUT_INTERVAL_PROP,
DEFAULT_TIMEOUT_INTERVAL);
- if (timeoutInterval > 0) {
- blobRequestOptions.setTimeoutIntervalInMs((int)
TimeUnit.SECONDS.toMillis(timeoutInterval));
- }
- }
+
+ public static RequestRetryOptions getRetryOptionsDefault() {
+ return getRetryOptionsDefault(null);
+ }
+
+ public static RequestRetryOptions getRetryOptionsDefault(String
secondaryHost) {
+ int retryAttempts = Integer.getInteger(RETRY_BACKOFF_PROP,
DEFAULT_RETRY_BACKOFF_SECONDS);
+ int timeoutExecution = Integer.getInteger(TIMEOUT_EXECUTION_PROP,
DEFAULT_TIMEOUT_EXECUTION);
+ int timeoutInterval = Integer.getInteger(TIMEOUT_INTERVAL_PROP,
DEFAULT_TIMEOUT_INTERVAL);
+ long timeoutIntervalToMs = timeoutInterval * 1_000L;
+ long timeoutIntervalMax = timeoutIntervalToMs * 5;
+
+ return new RequestRetryOptions(RetryPolicyType.EXPONENTIAL,
+ retryAttempts,
+ timeoutExecution,
+ timeoutIntervalToMs,
+ timeoutIntervalMax,
+ secondaryHost);
}
/**
- * Optimise the blob request options for write operations. This method
does not change the original blobRequestOptions.
- * This method also applies the default request options if they are not
already set, by calling {@link #applyDefaultRequestOptions(BlobRequestOptions)}
- * @param blobRequestOptions
- * @return write optimised blobRequestOptions
+ * secondaryHost is null because there is no writer in secondary
+ * @return
*/
- public static BlobRequestOptions
optimiseForWriteOperations(BlobRequestOptions blobRequestOptions) {
- BlobRequestOptions writeOptimisedBlobRequestOptions = new
BlobRequestOptions(blobRequestOptions);
- applyDefaultRequestOptions(writeOptimisedBlobRequestOptions);
-
- Integer writeTimeoutExecution =
Integer.getInteger(WRITE_TIMEOUT_EXECUTION_PROP);
- if (writeTimeoutExecution != null) {
- writeOptimisedBlobRequestOptions.setMaximumExecutionTimeInMs((int)
TimeUnit.SECONDS.toMillis(writeTimeoutExecution));
- }
-
- Integer writeTimeoutInterval =
Integer.getInteger(WRITE_TIMEOUT_INTERVAL_PROP);
- if (writeTimeoutInterval != null) {
- writeOptimisedBlobRequestOptions.setTimeoutIntervalInMs((int)
TimeUnit.SECONDS.toMillis(writeTimeoutInterval));
- }
-
- return writeOptimisedBlobRequestOptions;
+ public static RequestRetryOptions
getRetryOperationsOptimiseForWriteOperations() {
+ int retryAttempts = Integer.getInteger(RETRY_BACKOFF_PROP,
DEFAULT_RETRY_BACKOFF_SECONDS);
Review Comment:
```RETRY_BACKOFF_PROP``` does not denote the number of retries, rather the
time between consecutive retries
##########
oak-segment-azure/src/main/java/org/apache/jackrabbit/oak/segment/azure/util/AzureRequestOptions.java:
##########
@@ -45,52 +43,43 @@ public class AzureRequestOptions {
private AzureRequestOptions() {
}
- /**
- * Apply default request options to the blobRequestOptions if they are not
already set.
- * @param blobRequestOptions
- */
- public static void applyDefaultRequestOptions(BlobRequestOptions
blobRequestOptions) {
- if (blobRequestOptions.getRetryPolicyFactory() == null) {
- int retryAttempts = Integer.getInteger(RETRY_ATTEMPTS_PROP,
DEFAULT_RETRY_ATTEMPTS);
- if (retryAttempts > 0) {
- Integer retryBackoffSeconds =
Integer.getInteger(RETRY_BACKOFF_PROP, DEFAULT_RETRY_BACKOFF_SECONDS);
- blobRequestOptions.setRetryPolicyFactory(new
RetryLinearRetry((int) TimeUnit.SECONDS.toMillis(retryBackoffSeconds),
retryAttempts));
- }
- }
- if (blobRequestOptions.getMaximumExecutionTimeInMs() == null) {
- int timeoutExecution = Integer.getInteger(TIMEOUT_EXECUTION_PROP,
DEFAULT_TIMEOUT_EXECUTION);
- if (timeoutExecution > 0) {
- blobRequestOptions.setMaximumExecutionTimeInMs((int)
TimeUnit.SECONDS.toMillis(timeoutExecution));
- }
- }
- if (blobRequestOptions.getTimeoutIntervalInMs() == null) {
- int timeoutInterval = Integer.getInteger(TIMEOUT_INTERVAL_PROP,
DEFAULT_TIMEOUT_INTERVAL);
- if (timeoutInterval > 0) {
- blobRequestOptions.setTimeoutIntervalInMs((int)
TimeUnit.SECONDS.toMillis(timeoutInterval));
- }
- }
+
+ public static RequestRetryOptions getRetryOptionsDefault() {
+ return getRetryOptionsDefault(null);
+ }
+
+ public static RequestRetryOptions getRetryOptionsDefault(String
secondaryHost) {
+ int retryAttempts = Integer.getInteger(RETRY_BACKOFF_PROP,
DEFAULT_RETRY_BACKOFF_SECONDS);
+ int timeoutExecution = Integer.getInteger(TIMEOUT_EXECUTION_PROP,
DEFAULT_TIMEOUT_EXECUTION);
+ int timeoutInterval = Integer.getInteger(TIMEOUT_INTERVAL_PROP,
DEFAULT_TIMEOUT_INTERVAL);
+ long timeoutIntervalToMs = timeoutInterval * 1_000L;
+ long timeoutIntervalMax = timeoutIntervalToMs * 5;
+
+ return new RequestRetryOptions(RetryPolicyType.EXPONENTIAL,
+ retryAttempts,
+ timeoutExecution,
+ timeoutIntervalToMs,
+ timeoutIntervalMax,
+ secondaryHost);
}
/**
- * Optimise the blob request options for write operations. This method
does not change the original blobRequestOptions.
- * This method also applies the default request options if they are not
already set, by calling {@link #applyDefaultRequestOptions(BlobRequestOptions)}
- * @param blobRequestOptions
- * @return write optimised blobRequestOptions
+ * secondaryHost is null because there is no writer in secondary
+ * @return
*/
- public static BlobRequestOptions
optimiseForWriteOperations(BlobRequestOptions blobRequestOptions) {
- BlobRequestOptions writeOptimisedBlobRequestOptions = new
BlobRequestOptions(blobRequestOptions);
- applyDefaultRequestOptions(writeOptimisedBlobRequestOptions);
-
- Integer writeTimeoutExecution =
Integer.getInteger(WRITE_TIMEOUT_EXECUTION_PROP);
- if (writeTimeoutExecution != null) {
- writeOptimisedBlobRequestOptions.setMaximumExecutionTimeInMs((int)
TimeUnit.SECONDS.toMillis(writeTimeoutExecution));
- }
-
- Integer writeTimeoutInterval =
Integer.getInteger(WRITE_TIMEOUT_INTERVAL_PROP);
- if (writeTimeoutInterval != null) {
- writeOptimisedBlobRequestOptions.setTimeoutIntervalInMs((int)
TimeUnit.SECONDS.toMillis(writeTimeoutInterval));
- }
-
- return writeOptimisedBlobRequestOptions;
+ public static RequestRetryOptions
getRetryOperationsOptimiseForWriteOperations() {
+ int retryAttempts = Integer.getInteger(RETRY_BACKOFF_PROP,
DEFAULT_RETRY_BACKOFF_SECONDS);
+ Integer writeTimeoutExecution =
Integer.getInteger(WRITE_TIMEOUT_EXECUTION_PROP, DEFAULT_TIMEOUT_EXECUTION);
+ Integer writeTimeoutInterval =
Integer.getInteger(WRITE_TIMEOUT_INTERVAL_PROP, DEFAULT_TIMEOUT_INTERVAL);
+ long writeTimeoutIntervalToMs = writeTimeoutInterval * 1_000L;
+ long writeTimeoutIntervalMax = writeTimeoutIntervalToMs * 5;
+
+ return new RequestRetryOptions(RetryPolicyType.EXPONENTIAL,
Review Comment:
That is the behavior change. I would rather use RetryPolicyType.FIXED, since
SDK v8 was configured like that, and change the retry policy with another PR
if needed.
##########
oak-segment-azure/src/main/java/org/apache/jackrabbit/oak/segment/azure/util/AzureRequestOptions.java:
##########
@@ -45,52 +43,43 @@ public class AzureRequestOptions {
private AzureRequestOptions() {
}
- /**
- * Apply default request options to the blobRequestOptions if they are not
already set.
- * @param blobRequestOptions
- */
- public static void applyDefaultRequestOptions(BlobRequestOptions
blobRequestOptions) {
- if (blobRequestOptions.getRetryPolicyFactory() == null) {
- int retryAttempts = Integer.getInteger(RETRY_ATTEMPTS_PROP,
DEFAULT_RETRY_ATTEMPTS);
- if (retryAttempts > 0) {
- Integer retryBackoffSeconds =
Integer.getInteger(RETRY_BACKOFF_PROP, DEFAULT_RETRY_BACKOFF_SECONDS);
- blobRequestOptions.setRetryPolicyFactory(new
RetryLinearRetry((int) TimeUnit.SECONDS.toMillis(retryBackoffSeconds),
retryAttempts));
- }
- }
- if (blobRequestOptions.getMaximumExecutionTimeInMs() == null) {
- int timeoutExecution = Integer.getInteger(TIMEOUT_EXECUTION_PROP,
DEFAULT_TIMEOUT_EXECUTION);
- if (timeoutExecution > 0) {
- blobRequestOptions.setMaximumExecutionTimeInMs((int)
TimeUnit.SECONDS.toMillis(timeoutExecution));
- }
- }
- if (blobRequestOptions.getTimeoutIntervalInMs() == null) {
- int timeoutInterval = Integer.getInteger(TIMEOUT_INTERVAL_PROP,
DEFAULT_TIMEOUT_INTERVAL);
- if (timeoutInterval > 0) {
- blobRequestOptions.setTimeoutIntervalInMs((int)
TimeUnit.SECONDS.toMillis(timeoutInterval));
- }
- }
+
+ public static RequestRetryOptions getRetryOptionsDefault() {
+ return getRetryOptionsDefault(null);
+ }
+
+ public static RequestRetryOptions getRetryOptionsDefault(String
secondaryHost) {
+ int retryAttempts = Integer.getInteger(RETRY_BACKOFF_PROP,
DEFAULT_RETRY_BACKOFF_SECONDS);
+ int timeoutExecution = Integer.getInteger(TIMEOUT_EXECUTION_PROP,
DEFAULT_TIMEOUT_EXECUTION);
+ int timeoutInterval = Integer.getInteger(TIMEOUT_INTERVAL_PROP,
DEFAULT_TIMEOUT_INTERVAL);
+ long timeoutIntervalToMs = timeoutInterval * 1_000L;
+ long timeoutIntervalMax = timeoutIntervalToMs * 5;
+
+ return new RequestRetryOptions(RetryPolicyType.EXPONENTIAL,
+ retryAttempts,
+ timeoutExecution,
+ timeoutIntervalToMs,
Review Comment:
the intent of ```TIMEOUT_INTERVAL_PROP``` was to specify max execution time
for one retry
From the Java documentation
```
Sets the timeout to use when making this request.
The server timeout interval begins at the time that the complete request has
been received by the service, and the server begins processing the response. If
the timeout interval elapses before the response is returned to the client, the
operation times out. The timeout interval resets with each retry, if the
request is retried.
```
You can check the intent of the properties in the linked below nad map it to
the corresponding ones in v12
https://github.com/Azure/azure-storage-java/blob/v8.6.6/microsoft-azure-storage/src/com/microsoft/azure/storage/RequestOptions.java#L266
##########
oak-segment-azure/src/main/java/org/apache/jackrabbit/oak/segment/azure/util/AzureRequestOptions.java:
##########
@@ -45,52 +43,43 @@ public class AzureRequestOptions {
private AzureRequestOptions() {
}
- /**
- * Apply default request options to the blobRequestOptions if they are not
already set.
- * @param blobRequestOptions
- */
- public static void applyDefaultRequestOptions(BlobRequestOptions
blobRequestOptions) {
- if (blobRequestOptions.getRetryPolicyFactory() == null) {
- int retryAttempts = Integer.getInteger(RETRY_ATTEMPTS_PROP,
DEFAULT_RETRY_ATTEMPTS);
- if (retryAttempts > 0) {
- Integer retryBackoffSeconds =
Integer.getInteger(RETRY_BACKOFF_PROP, DEFAULT_RETRY_BACKOFF_SECONDS);
- blobRequestOptions.setRetryPolicyFactory(new
RetryLinearRetry((int) TimeUnit.SECONDS.toMillis(retryBackoffSeconds),
retryAttempts));
- }
- }
- if (blobRequestOptions.getMaximumExecutionTimeInMs() == null) {
- int timeoutExecution = Integer.getInteger(TIMEOUT_EXECUTION_PROP,
DEFAULT_TIMEOUT_EXECUTION);
- if (timeoutExecution > 0) {
- blobRequestOptions.setMaximumExecutionTimeInMs((int)
TimeUnit.SECONDS.toMillis(timeoutExecution));
- }
- }
- if (blobRequestOptions.getTimeoutIntervalInMs() == null) {
- int timeoutInterval = Integer.getInteger(TIMEOUT_INTERVAL_PROP,
DEFAULT_TIMEOUT_INTERVAL);
- if (timeoutInterval > 0) {
- blobRequestOptions.setTimeoutIntervalInMs((int)
TimeUnit.SECONDS.toMillis(timeoutInterval));
- }
- }
+
+ public static RequestRetryOptions getRetryOptionsDefault() {
+ return getRetryOptionsDefault(null);
+ }
+
+ public static RequestRetryOptions getRetryOptionsDefault(String
secondaryHost) {
+ int retryAttempts = Integer.getInteger(RETRY_BACKOFF_PROP,
DEFAULT_RETRY_BACKOFF_SECONDS);
Review Comment:
```RETRY_BACKOFF_PROP``` does not denote the number of retries, rather the
time between consecutive retries
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
