[core] Retry on 503 using the Retry-After header #15906

sadasant · 2021-06-22T23:53:26Z

Based on the Retry-After specification, 503 should also be supported when considering the Retry-After header.

This also aligns with upcoming Identity plans.

xirzec

LGTM 👍

sadasant · 2021-06-23T00:23:30Z

@xirzec wait, won't the exponentialRetryPolicy and the throttlingRetryPolicy overlap 🤔

azure-sdk-for-js/sdk/core/core-http/src/serviceClient.ts

Lines 736 to 738 in e823b26

    
           factories.push(exponentialRetryPolicy()); 
        
           factories.push(systemErrorRetryPolicy()); 
        
           factories.push(throttlingRetryPolicy());

like, the exponentialRetryPolicy will detect the 503s before the throttlingRetryPolicy 🤔

jeremymeng · 2021-06-23T16:37:49Z

@xirzec disregard! I understand now that lower in the code means that it will run first (since we wait for the next pipeline to run the current one).

We might want to remove 503 from exponentialRetryPolicy? Since we don't have total retry limit across retry policies, consecutive 503s could cause retires for both policies (not too bad, just taking longer to fail in the worst case).

xirzec · 2021-06-23T18:05:14Z

We might want to remove 503 from exponentialRetryPolicy? Since we don't have total retry limit across retry policies, consecutive 503s could cause retires for both policies (not too bad, just taking longer to fail in the worst case).

Yeah, I'm good with removing it from the other policy. We really need to consolidate these soon. 😄

sadasant · 2021-06-23T19:04:37Z

@xirzec @jeremymeng I’m writing some tests, I’ll write you directly after I push these changes

sadasant · 2021-06-24T18:13:08Z

sdk/core/core-http/src/policies/exponentialRetryPolicy.ts

+      response.headers.get(Constants.HeaderConstants.RETRY_AFTER)
+    ) {
+      return false;
+    }


I decided to put this in a separate conditional to make it easier to think through.

sadasant · 2021-06-24T18:13:57Z

sdk/core/core-http/src/util/throttlingRetryStrategy.ts

+/**
+ * Maximum number of retries for the throttling retry policy
+ */
+export const DEFAULT_CLIENT_MAX_RETRY_COUNT = 3;


I don’t believe we should preemptively expose this to the public API. If anything, now we retry up to 3 times on the throttlingRetryPolicy, while we were only retrying one time before.

Three is good, and is the same default value for exponential and system error retry policy.

sdk/identity/identity/test/public/node/clientSecretCredential.spec.ts

sdk/core/core-http/src/policies/throttlingRetryPolicy.ts

sdk/identity/identity/test/internal/node/managedIdentityCredential.spec.ts

…Async can't be awaited

xirzec

A few comments, some minor bugs, but looks good once those are addressed

sdk/core/core-http/src/policies/exponentialRetryPolicy.ts

sdk/core/core-http/test/policies/throttlingRetryPolicyTests.ts

sdk/core/core-rest-pipeline/src/policies/exponentialRetryPolicy.ts

sdk/core/core-rest-pipeline/src/policies/throttlingRetryPolicy.ts

sdk/core/core-rest-pipeline/test/throttlingRetryPolicy.spec.ts

…y.ts Co-authored-by: Jeff Fisher <xirzec@xirzec.com>

ghost · 2021-06-29T03:12:30Z

Hello @sadasant!

Because this pull request has the auto-merge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.

Based on the Retry-After specification, 503 should also be supported when considering the Retry-After header. This also aligns with upcoming Identity plans.

[core] Retry on 503 using the Retry-After header

7885d61

sadasant requested review from xirzec and jeremymeng June 22, 2021 23:53

ghost added the Azure.Core label Jun 22, 2021

xirzec approved these changes Jun 22, 2021

View reviewed changes

sadasant marked this pull request as ready for review June 22, 2021 23:56

sadasant requested review from chradek and deyaaeldeen as code owners June 22, 2021 23:56

This comment has been minimized.

Sign in to view

formatting

8ab4e31

core-http API review file

13a7ff9

sadasant requested a review from bterlson as a code owner June 23, 2021 03:17

WIP

0cc46d2

sadasant marked this pull request as draft June 23, 2021 19:46

sadasant removed request for bterlson, deyaaeldeen and chradek June 23, 2021 19:46

This comment has been minimized.

Sign in to view

sadasant and others added 2 commits June 23, 2021 23:29

the rest of the changes

efef18f

Merge branch 'main' into core/retry-on-503

da64440

sadasant requested a review from xirzec June 24, 2021 18:08

sadasant commented Jun 24, 2021

View reviewed changes

sdk/identity/identity/test/public/node/clientSecretCredential.spec.ts Outdated Show resolved Hide resolved

sadasant marked this pull request as ready for review June 24, 2021 18:15

sadasant requested a review from schaabs as a code owner June 24, 2021 18:15

sadasant requested a review from witemple-msft as a code owner June 24, 2021 18:15

Merge branch 'main' into core/retry-on-503

a2685f6

jeremymeng reviewed Jun 24, 2021

View reviewed changes

sdk/core/core-http/src/policies/throttlingRetryPolicy.ts Outdated Show resolved Hide resolved

sadasant added 2 commits June 25, 2021 10:57

Merge remote-tracking branch 'Azure/main' into core/retry-on-503

74bbb41

temporary fix for Node 16

fdd9ecf

jeremymeng reviewed Jun 25, 2021

View reviewed changes

sdk/identity/identity/test/internal/node/managedIdentityCredential.spec.ts Outdated Show resolved Hide resolved

sadasant added 2 commits June 25, 2021 18:17

feedback by Jeremy

b561d19

Since unhandled promise rejections are not supported in Node 16, tick…

33260ec

…Async can't be awaited

sadasant force-pushed the core/retry-on-503 branch from b215086 to 33260ec Compare June 25, 2021 19:01

sadasant added 3 commits June 25, 2021 19:06

some comment improvements

502f682

Merge remote-tracking branch 'Azure/main' into core/retry-on-503

0127649

small improvement

023ce55

sadasant requested a review from jeremymeng June 28, 2021 13:50

xirzec approved these changes Jun 28, 2021

View reviewed changes

Update sdk/core/core-rest-pipeline/src/policies/exponentialRetryPolic…

8b7fa25

…y.ts Co-authored-by: Jeff Fisher <xirzec@xirzec.com>

ramya-rao-a mentioned this pull request Jun 29, 2021

[Identity] [Hotfix] 1.4.0 increased the IMDS MSI default timeout #16055

Merged

sadasant added 2 commits June 29, 2021 01:42

core-http feedback from Jeff, and tests

3539c30

the rest of the feedback, and test improvements

a6636a1

sadasant force-pushed the core/retry-on-503 branch from 7f5e607 to a6636a1 Compare June 29, 2021 03:09

sadasant added the auto-merge label Jun 29, 2021

ghost merged commit c8126be into Azure:main Jun 29, 2021

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[core] Retry on 503 using the Retry-After header #15906

[core] Retry on 503 using the Retry-After header #15906

sadasant commented Jun 22, 2021

xirzec left a comment

This comment has been minimized.

sadasant commented Jun 23, 2021

jeremymeng commented Jun 23, 2021 •

edited

Loading

xirzec commented Jun 23, 2021

sadasant commented Jun 23, 2021

This comment has been minimized.

sadasant Jun 24, 2021

sadasant Jun 24, 2021

jeremymeng Jun 25, 2021

xirzec left a comment

ghost commented Jun 29, 2021

[core] Retry on 503 using the Retry-After header #15906

[core] Retry on 503 using the Retry-After header #15906

Conversation

sadasant commented Jun 22, 2021

xirzec left a comment

Choose a reason for hiding this comment

This comment has been minimized.

sadasant commented Jun 23, 2021

jeremymeng commented Jun 23, 2021 • edited Loading

xirzec commented Jun 23, 2021

sadasant commented Jun 23, 2021

This comment has been minimized.

sadasant Jun 24, 2021

Choose a reason for hiding this comment

sadasant Jun 24, 2021

Choose a reason for hiding this comment

jeremymeng Jun 25, 2021

Choose a reason for hiding this comment

xirzec left a comment

Choose a reason for hiding this comment

ghost commented Jun 29, 2021

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

jeremymeng commented Jun 23, 2021 •

edited

Loading

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.