-
Notifications
You must be signed in to change notification settings - Fork 1.4k
[core-amqp] Implement exponential retry mechanism #4175
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
926be21
b49cbd3
75c8caa
bb95538
24ba04b
e72d3a2
2c02e5c
3540491
324a868
e65ea10
033be9a
493da07
aa0d384
8af8f8d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4,7 +4,12 @@ | |
| import { translate, MessagingError } from "./errors"; | ||
| import { delay, isNode } from "./util/utils"; | ||
| import * as log from "./log"; | ||
| import { defaultMaxRetries, defaultDelayBetweenRetriesInSeconds } from "./util/constants"; | ||
| import { | ||
| defaultMaxRetries, | ||
| defaultDelayBetweenOperationRetriesInSeconds, | ||
| defaultMaxDelayForExponentialRetryInMs, | ||
| defaultMinDelayForExponentialRetryInMs | ||
| } from "./util/constants"; | ||
| import { resolve } from "dns"; | ||
|
|
||
| /** | ||
|
|
@@ -25,6 +30,15 @@ function isDelivery(obj: any): boolean { | |
| return result; | ||
| } | ||
|
|
||
| /** | ||
| * Describes the RetryPolicy type | ||
| * @enum RetryPolicy | ||
| */ | ||
| export enum RetryPolicy { | ||
| ExponentialRetryPolicy, | ||
| LinearRetryPolicy | ||
| } | ||
|
|
||
| /** | ||
| * Describes the retry operation type. | ||
| * @enum RetryOperationType | ||
|
|
@@ -66,14 +80,30 @@ export interface RetryConfig<T> { | |
| maxRetries?: number; | ||
| /** | ||
| * @property {number} [delayInSeconds] Amount of time to wait in seconds before making the | ||
| * next attempt. Default: 15. | ||
| * next attempt. Default: 30. | ||
| * When `retryPolicy` option is set to `ExponentialRetryPolicy`, \ | ||
| * this is used to compute the exponentially increasing delays between retries. | ||
| */ | ||
| delayInSeconds?: number; | ||
| /** | ||
| * @property {string} connectionHost The host "<yournamespace>.servicebus.windows.net". | ||
| * Used to check network connectivity. | ||
| */ | ||
| connectionHost?: string; | ||
| /** | ||
| * @property {RetryPolicy} [retryPolicy] Denotes which retry policy to apply. Default is `LinearRetryPolicy` | ||
| */ | ||
| retryPolicy?: RetryPolicy; | ||
| /** | ||
| * @property {number} [maxExponentialRetryDelayInMs] Denotes the maximum delay between retries | ||
| * that the retry attempts will be capped at. Applicable only when performing exponential retry. | ||
| */ | ||
| maxExponentialRetryDelayInMs?: number; | ||
| /** | ||
| * @property {number} [minExponentialRetryDelayInMs] Denotes the minimum delay between retries | ||
| * to use. Applicable only when performing exponential retry. | ||
| */ | ||
| minExponentialRetryDelayInMs?: number; | ||
| } | ||
|
|
||
| /** | ||
|
|
@@ -115,10 +145,13 @@ async function checkNetworkConnection(host: string): Promise<boolean> { | |
| * The number of additional attempts is governed by the `maxRetries` property provided | ||
| * on the `RetryConfig` argument. | ||
| * | ||
| * The retries when made are done so linearly on the given operation for a specified number of times, | ||
| * with a specified delay in between each retry. | ||
| * If `retryPolicy` option is set to `LinearRetryPolicy`, then the retries when made are done so linearly on the | ||
| * given operation for a specified number of times, with a specified delay in between each retry. | ||
| * | ||
| * If `retryPolicy` option is set to `ExponentialRetryPolicy`, then the delay between retries is adjusted to increase | ||
| * exponentially with each attempt using back-off factor of power 2. | ||
| * | ||
| * @param {RetryConfig<T>} config Parameters to configure retry operation. | ||
| * @param {RetryConfig<T>} config Parameters to configure retry operation | ||
| * | ||
| * @return {Promise<T>} Promise<T>. | ||
| */ | ||
|
|
@@ -128,7 +161,13 @@ export async function retry<T>(config: RetryConfig<T>): Promise<T> { | |
| config.maxRetries = defaultMaxRetries; | ||
| } | ||
| if (config.delayInSeconds == undefined || config.delayInSeconds < 0) { | ||
| config.delayInSeconds = defaultDelayBetweenRetriesInSeconds; | ||
| config.delayInSeconds = defaultDelayBetweenOperationRetriesInSeconds; | ||
| } | ||
| if (config.maxExponentialRetryDelayInMs == undefined || config.maxExponentialRetryDelayInMs < 0) { | ||
| config.maxExponentialRetryDelayInMs = defaultMaxDelayForExponentialRetryInMs; | ||
| } | ||
| if (config.minExponentialRetryDelayInMs == undefined || config.minExponentialRetryDelayInMs < 0) { | ||
| config.minExponentialRetryDelayInMs = defaultMinDelayForExponentialRetryInMs; | ||
| } | ||
| let lastError: MessagingError | undefined; | ||
| let result: any; | ||
|
|
@@ -174,14 +213,28 @@ export async function retry<T>(config: RetryConfig<T>): Promise<T> { | |
| i, | ||
| err | ||
| ); | ||
| let targetDelayInMs = config.delayInSeconds; | ||
| if (config.retryPolicy === RetryPolicy.ExponentialRetryPolicy) { | ||
| let incrementDelta = Math.pow(2, i) - 1; | ||
| const boundedRandDelta = | ||
| config.delayInSeconds * 0.8 + | ||
| Math.floor(Math.random() * (config.delayInSeconds * 1.2 - config.delayInSeconds * 0.8)); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Isn't
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Right, maybe the magic numbers/factors have a certain meaning? For instance we have time in milliseconds (say for 30 sec) being expressed as 1000 * 30 in some places for readability. |
||
| incrementDelta *= boundedRandDelta; | ||
|
|
||
| targetDelayInMs = Math.min( | ||
| config.minExponentialRetryDelayInMs + incrementDelta, | ||
| config.maxExponentialRetryDelayInMs | ||
| ); | ||
| } | ||
|
|
||
| if (lastError && lastError.retryable) { | ||
| log.error( | ||
| "[%s] Sleeping for %d seconds for '%s'.", | ||
| config.connectionId, | ||
| config.delayInSeconds, | ||
| targetDelayInMs / 1000, | ||
| config.operationType | ||
| ); | ||
| await delay(config.delayInSeconds * 1000); | ||
| await delay(targetDelayInMs); | ||
| continue; | ||
| } else { | ||
| break; | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At a quick glance, the words
minExponentialRetryDelayInMsandmaxExponentialRetryDelayInMstell me that when using exponential retry policy, the delay between retries starts from theminExponentialRetryDelayInMsand is incremented exponentially tillmaxExponentialRetryDelayInMsThis understanding holds true only if
incrementDeltais initialized toMath.pow(2, i - 1) - 1instead ofMath.pow(2, i) - 1But, I am not sure if this what we want it to mean :)
i.e I am not sure if the first delay interval should be
minExponentialRetryDelayInMsorminExponentialRetryDelayInMs + (a random value between 0 and 1 * delayInSeconds)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Resolution to this is not a blocker for merging this PR, but is a blocker in closing the linked issue
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bterlson ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with your intuition here - the first time we retry, incrementDelta should be zero, and since we start on "try 1",
Math.pow(2, i - 1) - 1makes sense to me.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In that case
minExponentialRetryDelayInMswould need a better default ... Logged #4201 to discuss this