Add auto-retry logic for configurable list of error codes #94
Conversation
| @@ -90,6 +90,16 @@ function Initialize-StoreIngestionApiGlobalVariables | |||
| { | |||
| $global:SBDefaultProxyEndpoint = $null | |||
| } | |||
|
|
|||
| if (!(Get-Variable -Name SBAutoRetryErrorCodes -Scope Global -ValueOnly -ErrorAction Ignore)) | |||
There was a problem hiding this comment.
This isn't the place for it, but our handling of global variables could easily be implemented generically, with a hashtable of variable names and default values.
There was a problem hiding this comment.
Interesting idea. That would work for most of them (immediate exception that I see is how $SBLogPath and $SBLoggingEnabled in Helpers.ps1 have some interaction during initialization). I've opened #95 to track this suggestion for future consideration.
StoreBroker/StoreIngestionApi.psm1
Outdated
| { | ||
| $headers.Add("UseINT", "true") | ||
| $AccessToken = Get-AccessToken -NoStatus:$NoStatus |
There was a problem hiding this comment.
What's the reasoning for this being in the loop?
There was a problem hiding this comment.
AccessTokens have a limited lifetime. If we didn't get a new one each time, it's possible that it would be invalid during one of the retry attempts.
There was a problem hiding this comment.
Could you grab your comment and add it to the script?
There was a problem hiding this comment.
Good feedback. Will do.
StoreBroker/StoreIngestionApi.psm1
Outdated
| } | ||
|
|
||
| # Since we have retry logic, we won't create a new stopwatch every time, | ||
| # we'll continue the existing one if it already exists... |
There was a problem hiding this comment.
nit: I'd remove the 'if' because $stopwatch always exists
There was a problem hiding this comment.
Agreed. That comment was from a previous iteration
There is a set of known error codes where users can usually achieve success if they simply retry enough times:
* `429` - The Submission API limits a Tenant to 20 requests per minute. Any requests exceeding
that will receive this error code until that minute has expired.
* `503` - An underlying service component was unavailable, and the Submission API suggests that
you try again.
This change modifies the default execution behavior of StoreBroker to auto-retry up to **5** times with exponential back-off for these two error codes. The implementation adds two new global variables to permit users to further configure how this works:
**`$global:SBAutoRetryErrorCodes`** - an `int` list of status codes that we should retry for
**`$global:SBMaxAutoRetries`** - an `int` indicating the maximum number of times we should retry before failing.
We will only report back telemetry when the retry count is reached (and we finally error out), or when the result is finally successful. We are adding two new telemetry properties that will be reported for both (eventual) successful requests,, as well as for eventual exceptions:
* `NumRetries` - to track the number of times we had to attempt a retry
* `RetryStatusCode` - to track the last status code that was hit that caused us to need to do the retry.
Resolves microsoft#92
HowardWolosky
force-pushed
the
429retry
branch
from
b4d799c to
06fba04
Compare
There is a set of known error codes where users can usually achieve success if they simply retry enough times:
429- The Submission API limits a Tenant to 20 requests per minute. Any requests exceedingthat will receive this error code until that minute has expired.
503- An underlying service component was unavailable, and the Submission API suggests thatyou try again.
This change modifies the default execution behavior of StoreBroker to auto-retry up to 5 times with exponential back-off for these two error codes. The implementation adds two new global variables to permit users to further configure how this works:
$global:SBAutoRetryErrorCodes- anintlist of status codes that we should retry for$global:SBMaxAutoRetries- anintindicating the maximum number of times we should retry before failing.We will only report back telemetry when the retry count is reached (and we finally error out), or when the result is finally successful. We are adding two new telemetry properties that will be reported for both (eventual) successful requests,, as well as for eventual exceptions:
NumRetries- to track the number of times we had to attempt a retryRetryStatusCode- to track the last status code that was hit that caused us to need to do the retry.Resolves #92