[core] Unify the ThrottlingRetryPolicy in core and app-configuration

[richardpark-msft]

As of PR #6431 we now have two implementations of the ThrottlingRetryPolicy - one in core-http and one in AppConfiguration.

We probably want to discuss how to properly merge them in a way that is non-breaking.

The differences between them are:

.catch vs .then:

• AppConfig's ThrottlingRetryPolicy attaches itself to the .catch of the returned promise
  (internally the HTTP client generates a 429 which causes a RestError to be thrown).
  • Is this different than what other clients are getting? Is AppConfig doing something incorrect here?
• core-http attaches itself using .then which doesn't fire off for appconfig

headers

• AppConfig's policy checks Retry-After and two additional headers (although we probably only need to check one)
• core-http's only checks Retry-After

I have enough tests that this is simple enough to swap out and ensure it works in AppConfig but I'm unsure about the rest of the codebase.

In both cases the throttling retry policy doesn't have a retry limit. We probably want to add that in while we're working on this.

[richardpark-msft]

CC: @ramya-rao-a , @johanste , @bterlson , @daviwil

[bterlson]

As mentioned in another thread, I think the distinction between catch/then handling in the policy is likely not intentional. It's possible appconfig is doing a weird thing by mapping the 429 error code, but it seems reasonable to me and so I think the policy should handle both .catch and .then. But I would like @johanste to weigh in once he's back tomorrow (I think?).

[daviwil]

It looks like core-http's ThrottlingRetryPolicy is also looking for StatusCodes.TooManyRequests in responses so if that status only ever gets returned as a Promise rejection then we need to add .catch() logic there anyway to support it correctly.

[richardpark-msft]

Adding @xiangyan99 - looks like Python might need to change something as well, so just keeping him apprised of what's going on here.

[ramya-rao-a]

@xirzec, @jeremymeng Do you think this should be part of v1 or v2 of core-http?

[xirzec]

I think this works better in v2 personally

[jeremymeng]

Agree it's better to do as part of v2.

[jeremymeng]

We'd like to have a unified retry policy that allows some degree of customization.

Here's what other languages are doing. They all have a single retry policy.

• Java

  Java has a single retry policy which handles retrying on network errors and specific status codes. Underneath that we have retrying strategies, fixed and exponential backoff as implementations but a customer could provide their own, where the maximum retry count is configured for the specific retry policy.

• .NET

  .NET has a single retry policy that doesn’t allow plugging in retry strategies. The retry count is configured on retry options.

• Python

  Python has a single retry policy
  We expose some configurable settings including total_retry, retry_mode, retry_on_status_codes, etc.

• C++

  C++ also has a single retry policy, with retry options for max retries, what status codes are retriable, etc.
  We have a way (via the context) to track and expose the RetryCount from within the policy that other internal policies like storage’s (switch to secondary host) can use to keep track of how many retries have happened.
  Someone can derive from that retry policy to override whether to retry on a transport failure, and which response status codes to retry on, if needed.

[sadasant]

Now that the retry policies have been refactored to re-use key components under a new unified retryPolicy, we will be able to reduce most of the retry code on App Config.

How it would happen is that App Config’s throttling try policy here: https://github.com/Azure/azure-sdk-for-js/blob/main/sdk/appconfiguration/app-configuration/src/policies/throttlingRetryPolicy.ts would be written similar to how the throttling retry policy is written in the new core, here: https://github.com/Azure/azure-sdk-for-js/blob/main/sdk/core/core-rest-pipeline/src/policies/throttlingRetryPolicy.ts#L38-L45

However, we will only be able to do this after App Config migrates to the new core: #15809 which is scheduled for the February Milestone. Therefore, I’m moving this issue to the February Milestone as well.

[sadasant]

I haven’t seen updates on #15809 , so I’m moving this to the next milestone.

[sadasant]

Moving it two milestones ahead to give #15809 some time.

[HarshaNalluru]

#20817