Skip to content
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

adding error codes in retry logic #7107

Merged
merged 1 commit into from
May 4, 2018

Conversation

SumiranAgg
Copy link
Contributor

No description provided.

@SumiranAgg SumiranAgg requested a review from vincent1173 April 30, 2018 14:07
Copy link
Contributor

@vincent1173 vincent1173 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please increase the patch version for Azure App Service Deploy Task & Azure Resource Group Deployment.

@vincent1173 vincent1173 added Area: Release Area: AzureAppService Label to monitor Azure App Service issues labels Apr 30, 2018
@@ -21,14 +21,8 @@ export class KuduServiceManagementClient {
request.headers = request.headers || {};
request.headers["Authorization"] = "Basic " + this._accesssToken;
request.headers['Content-Type'] = 'application/json; charset=utf-8';
var options: webClient.WebRequestOptions = {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why remove this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here we were adding a default list of error codes which we have now added in WebClient directly.

let retriableErrorCodes = options && options.retriableErrorCodes ? options.retriableErrorCodes : ["ETIMEDOUT", "ECONNRESET"];
let retriableStatusCodes = options && options.retriableStatusCodes ? options.retriableStatusCodes: [];
let retriableErrorCodes = options && options.retriableErrorCodes ? options.retriableErrorCodes : ["ETIMEDOUT", "ECONNRESET", "ENOTFOUND", "ESOCKETTIMEDOUT", "ECONNREFUSED", "EHOSTUNREACH", "EPIPE", "EA_AGAIN"];
let retriableStatusCodes = options && options.retriableStatusCodes ? options.retriableStatusCodes: [408, 409, 500, 502, 503, 504];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

are 409, 500 retriable in all cases?

Copy link
Contributor

@vincent1173 vincent1173 May 2, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sachinma
409 - conflict happens, when multiple calls made to same resource, it sends conflict message and asks to retry after some time. (It is reliable to retry)
500 - we added this, because, many of the API calls that returned 500 passes in next retry. (for instance, Swap API monitor API failed with 500 in monitoring call, but it was passed in Azure side, so we considered retrying for 500. One more instance is with Update Application settings API, where the first call failed but second retry passed). 500 is not retriable in all cases, but given the number of instances, it passed in next retry, we took this path

@SumiranAgg SumiranAgg merged commit 2d79552 into master May 4, 2018
@SumiranAgg SumiranAgg deleted the users/suaggar/addErrorCodesForretry branch May 4, 2018 07:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: AzureAppService Label to monitor Azure App Service issues Area: Release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants