[App Config] Throttling retry policy update - no retrying forever by HarshaNalluru · Pull Request #16376 · Azure/azure-sdk-for-js

HarshaNalluru · 2021-07-14T03:03:09Z

Issue #16366

In this PR

Extending AppConfig client options with retry options with only maxRetries and the maxRetryDelay instead of all the four options of the RetryOptions interface in core-http
Update the custom throttling policy in app-config to not retry forever
And to make throttling retry policy honour maxRetries/maxRetryDelay

Fixes #16366

… harshan/app-config/issue/16366

sdk/appconfiguration/app-configuration/CHANGELOG.md

ramya-rao-a · 2021-07-14T16:55:58Z

sdk/appconfiguration/app-configuration/package.json

  "author": "Microsoft Corporation",
  "description": "An isomorphic client library for the Azure App Configuration service.",
-  "version": "1.2.0",
+  "version": "1.2.1",


Why didnt the automation that increments version number do this?

I couldn't find it yesterday.

I was filtering with App Configuration label and it turned out there was no label on the automated PR #16269.

Asked @praveenkuttappan in the teams' channel

… harshan/app-config/issue/16366

…arshaNalluru/azure-sdk-for-js into harshan/app-config/issue/16366

sdk/appconfiguration/app-configuration/CHANGELOG.md

sdk/appconfiguration/app-configuration/review/app-configuration.api.md

chradek · 2021-07-14T17:27:00Z

Extending AppConfig client options with retry options with only maxRetries and the maxRetryDelay instead of all the four options of the RetryOptions interface in core-http

Why is retryDelayInMs excluded?

And to make throttling retry policy honour maxRetries/maxRetryDelay

I feel like this has been discussed, but is there an issue tracking honoring the retry count across retry policies (or consolidating retry policies into a single one that can keep track of the retry count)?

jeremymeng · 2021-07-14T17:49:29Z

sdk/appconfiguration/app-configuration/src/policies/throttlingRetryPolicy.ts

-        if (delayInMs == null) {
+        if (
+          delayInMs == null ||
+          (this.retryOptions.maxRetryDelayInMs && delayInMs > this.retryOptions.maxRetryDelayInMs)


should we just delay(maxRetryDelayInMs) for the case of (this.retryOptions.maxRetryDelayInMs && delayInMs > this.retryOptions.maxRetryDelayInMs)?

Makes sense.

HarshaNalluru · 2021-07-14T17:51:46Z

Why is retryDelayInMs excluded?

retryDelayInMs is not for the throttlingRetryPolicy since we get the delay in ms from the error response from the service.

Eventually, we'll unify the retry policies in core.
Eventually, app-config will reuse what's present in core.

For that to happen, core has to allow/support parsing the "different" headers in the error response to get the delay in ms from the app-config service.
And core has to support retrying for the user-configured number of retries similar to what this PR does.

This current PR only cares about the throttling policy and is meant to unblock the customer.

I feel like this has been discussed, but is there an issue tracking honoring the retry count across retry policies (or consolidating retry policies into a single one that can keep track of the retry count)?

@chradek We'll get to that soon.. #9569

…arshaNalluru/azure-sdk-for-js into harshan/app-config/issue/16366

…elayInMs && delayInMs > this.retryOptions.maxRetryDelayInMs)

jeremymeng · 2021-07-14T21:17:57Z

sdk/appconfiguration/app-configuration/src/policies/throttlingRetryPolicy.ts

 */
-export function throttlingRetryPolicy(): RequestPolicyFactory {
+export function throttlingRetryPolicy(
+  retryOptions?: Pick<RetryOptions, "maxRetries" | "maxRetryDelayInMs">


update the parameter type to use the local one?

jeremymeng · 2021-07-14T21:18:21Z

sdk/appconfiguration/app-configuration/src/policies/throttlingRetryPolicy.ts

+  constructor(
+    nextPolicy: RequestPolicy,
+    options: RequestPolicyOptions,
+    private retryOptions: Pick<RetryOptions, "maxRetries" | "maxRetryDelayInMs"> = {}


update the parameter type to use the local one?

jeremymeng · 2021-07-14T21:29:41Z

sdk/appconfiguration/app-configuration/src/policies/throttlingRetryPolicy.ts

        }

+        if (
+          this.retryOptions.maxRetryDelayInMs &&


can we set this to the default in the constructor default option value? Otherwise the delay can go unbounded.

private retryOptions: Pick<RetryOptions, "maxRetries" | "maxRetryDelayInMs"> = { maxRetryDelayInMs: 90*1000 // is this constant defined somewhere? maxRetries: DEFAULT_CLIENT_RETRY_COUNT }

This is fine for maxRetries, will add that.
I don't want to define maxRetryDelayInMs since different services may have different values.

For example, with the observations from app-config, I have seen values anywhere between 20ms and 300000ms.

For the throttlign policy case, if the error from the service is with the delay, then we use it for delay and throw directly otherwise. I don't think there is a case of unboundedness.

maxRetryDelayInMs should only be provided by the user according to me.

Sounds fair. Please update the ref doc for maxRetryDelayInMs to remove the mentioning of default value.

jeremymeng

one comment about the ref doc. otherwise looking good!

…6376) * app-config throttlign retry policy fix - no retrying forever * typo * update versions * Add changelog * Update sdk/appconfiguration/app-configuration/CHANGELOG.md * Update sdk/appconfiguration/app-configuration/CHANGELOG.md * Redeclare RetryOptions * API Report * delay(maxRetryDelayInMs) for the case of (this.retryOptions.maxRetryDelayInMs && delayInMs > this.retryOptions.maxRetryDelayInMs) * address feedback * remove 90 seconds default

HarshaNalluru added 2 commits July 14, 2021 02:58

app-config throttlign retry policy fix - no retrying forever

6066532

typo

726bf80

HarshaNalluru requested review from bterlson, chradek, richardpark-msft and xirzec as code owners July 14, 2021 03:03

ghost added the App Configuration Azure.ApplicationModel.Configuration label Jul 14, 2021

HarshaNalluru requested a review from jeremymeng July 14, 2021 03:03

HarshaNalluru added 3 commits July 14, 2021 03:05

Merge branch 'main' of https://github.com/Azure/azure-sdk-for-js into…

37fdf20

… harshan/app-config/issue/16366

update versions

4cdb12c

Add changelog

037f993

HarshaNalluru requested review from ramya-rao-a and sadasant July 14, 2021 03:18

HarshaNalluru commented Jul 14, 2021

View reviewed changes

sdk/appconfiguration/app-configuration/CHANGELOG.md Outdated Show resolved Hide resolved

Update sdk/appconfiguration/app-configuration/CHANGELOG.md

2db9c69

ramya-rao-a reviewed Jul 14, 2021

View reviewed changes

HarshaNalluru added 2 commits July 14, 2021 17:03

Merge branch 'main' of https://github.com/Azure/azure-sdk-for-js into…

2c65ba2

… harshan/app-config/issue/16366

Merge branch 'harshan/app-config/issue/16366' of https://github.com/H…

2879263

…arshaNalluru/azure-sdk-for-js into harshan/app-config/issue/16366

HarshaNalluru commented Jul 14, 2021

View reviewed changes

sdk/appconfiguration/app-configuration/CHANGELOG.md Outdated Show resolved Hide resolved

Update sdk/appconfiguration/app-configuration/CHANGELOG.md

feefe53

sadasant reviewed Jul 14, 2021

View reviewed changes

sdk/appconfiguration/app-configuration/review/app-configuration.api.md Outdated Show resolved Hide resolved

jeremymeng reviewed Jul 14, 2021

View reviewed changes

HarshaNalluru added 4 commits July 14, 2021 20:40

Redeclare RetryOptions

763b696

API Report

98be9a5

Merge branch 'harshan/app-config/issue/16366' of https://github.com/H…

3d4b51e

…arshaNalluru/azure-sdk-for-js into harshan/app-config/issue/16366

delay(maxRetryDelayInMs) for the case of (this.retryOptions.maxRetryD…

18c30c7

…elayInMs && delayInMs > this.retryOptions.maxRetryDelayInMs)

jeremymeng reviewed Jul 14, 2021

View reviewed changes

address feedback

8cbb6b7

chradek approved these changes Jul 14, 2021

View reviewed changes

HarshaNalluru requested review from jeremymeng, ramya-rao-a and sadasant July 14, 2021 23:04

jeremymeng approved these changes Jul 14, 2021

View reviewed changes

remove 90 seconds default

99bbd7f

HarshaNalluru enabled auto-merge (squash) July 15, 2021 18:04

HarshaNalluru merged commit bf2b891 into Azure:main Jul 15, 2021

HarshaNalluru deleted the harshan/app-config/issue/16366 branch August 4, 2021 20:54

Conversation

HarshaNalluru commented Jul 14, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Issue #16366

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

chradek commented Jul 14, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

HarshaNalluru commented Jul 14, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

HarshaNalluru Jul 14, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jeremymeng left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

HarshaNalluru commented Jul 14, 2021 •

edited

Loading

HarshaNalluru commented Jul 14, 2021 •

edited

Loading

HarshaNalluru Jul 14, 2021 •

edited

Loading