-
Notifications
You must be signed in to change notification settings - Fork 59
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
fix: Always set the retry attempt to 0 for now #1251
fix: Always set the retry attempt to 0 for now #1251
Conversation
If the retry attempt is greater than 1 then a library called retryRequest creates a large delay. If we force the retry count to be 0 then we skip this delay and the requests run in the required timeline.
…b.com/danieljbruce/nodejs-bigtable into set-number-of-consecutive-errors-to-0
@@ -776,7 +776,7 @@ Please use the format 'prezzy' or '${instance.name}/tables/prezzy'.`); | |||
} as google.bigtable.v2.IReadRowsRequest; | |||
|
|||
const retryOpts = { | |||
currentRetryAttempt: numConsecutiveErrors, | |||
currentRetryAttempt: 0, // was numConsecutiveErrors |
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.
Do you know why "numConsecutiveErrors" doesn't work here even though its value is 0 according to Line 730?
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.
numConsecutiveErrors
works, but if it is greater than 0 then that will make the request take longer. Note that the number of consecutive errors is incremented at
Line 924 in 44fab5c
numConsecutiveErrors++; |
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 found this
calculatedNextRetryDelay = config.initialRetryDelayMillis * Math.pow(config.retryDelayMultiplier, numConsecutiveErrors)
It implies that when it's executed for the first time (i.e. the original request has failure), numConsecutiveErrors
should be 0 such that the retry delay is config.initialRetryDelayMillis
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.
Keep in mind that the change in the PR doesn't affect this code. This is a delay that happens in the veneer layer while the delay in retryRequest happens below the veneer layer.
@@ -776,7 +776,7 @@ Please use the format 'prezzy' or '${instance.name}/tables/prezzy'.`); | |||
} as google.bigtable.v2.IReadRowsRequest; | |||
|
|||
const retryOpts = { | |||
currentRetryAttempt: numConsecutiveErrors, | |||
currentRetryAttempt: 0, // was numConsecutiveErrors |
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 found this
calculatedNextRetryDelay = config.initialRetryDelayMillis * Math.pow(config.retryDelayMultiplier, numConsecutiveErrors)
It implies that when it's executed for the first time (i.e. the original request has failure), numConsecutiveErrors
should be 0 such that the retry delay is config.initialRetryDelayMillis
If the retry attempt is greater than 1 then a library called retryRequest creates a large delay. If we force the retry count to be 0 then we skip this delay and the requests run in the required timeline.