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

[App Config] Expose retryOptions from app-config to make it configurable #16366

Closed
HarshaNalluru opened this issue Jul 13, 2021 · 4 comments · Fixed by #16376
Closed

[App Config] Expose retryOptions from app-config to make it configurable #16366

HarshaNalluru opened this issue Jul 13, 2021 · 4 comments · Fixed by #16376
Assignees
Labels
App Configuration Azure.ApplicationModel.Configuration Client This issue points to a problem in the data-plane of the library.
Milestone

Comments

@HarshaNalluru
Copy link
Member

HarshaNalluru commented Jul 13, 2021

TODO:

  • Add retryOptions to the client options to make it configurable

Things to keep in mind:

  • custom throttling policy is different from what the core-http offers
  • throttling policy should play well with the retryOptions

Ask from Maryanne Gichohi (internal customer - App config team)

@ghost ghost added the needs-triage Workflow: This is a new issue that needs to be triaged to the appropriate team. label Jul 13, 2021
@HarshaNalluru HarshaNalluru added the App Configuration Azure.ApplicationModel.Configuration label Jul 13, 2021
@ghost ghost removed the needs-triage Workflow: This is a new issue that needs to be triaged to the appropriate team. label Jul 13, 2021
@HarshaNalluru HarshaNalluru added Client This issue points to a problem in the data-plane of the library. needs-triage Workflow: This is a new issue that needs to be triaged to the appropriate team. labels Jul 13, 2021
@ghost ghost removed the needs-triage Workflow: This is a new issue that needs to be triaged to the appropriate team. label Jul 13, 2021
@HarshaNalluru HarshaNalluru self-assigned this Jul 13, 2021
@HarshaNalluru HarshaNalluru added this to the [2021] August milestone Jul 13, 2021
@HarshaNalluru
Copy link
Member Author

HarshaNalluru commented Jul 13, 2021

Had a chat with @jeremymeng.

In conclusion, I'm going with the following approach,

  • I'll extend the 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 honour maxRetries/maxRetryDelay

More thoughts

  • core-http and core-rest-pipeline should also follow the similar pattern
  • Eventually, we should consolidate the {system/exponential/throttling}RetryPolicies into one.

/cc @xirzec @ramya-rao-a

@ramya-rao-a
Copy link
Contributor

Can we treat #6484 and #9569 as pre-requisites before we invest further in the specific retry logic for App Config?

@HarshaNalluru
Copy link
Member Author

@ramya-rao-a
Internal customer(app-config team) is waiting on it.
We do have a clear idea of what to do for App Config, which is in sync with what we have duplicated in core-http.
Coming to the differences, app-config should ideally only differ with core-http's implementation w.r.t the header we pick for the retry-after-ms.

TODO in app-config

  • Update the throttling policy to not retry forever
  • Update the throttling policy to use maxRetries
  • Update the throttling policy to use maxRetryDelay

TODO in core-http

  • throttling policy uses hard-coded retries instead of user-configured maxRetries, this needs to be changed to use maxRetries
  • Update the throttling policy to use maxRetryDelay

TODO in core-rest-pipeline

  • Update the throttling policy to use maxRetries
  • Update the throttling policy to use maxRetryDelay
  • Update the delay to use abortSignal

Whatever we do in app-config should also be done in core-http and core-rest-pipeline.
All three libraries would have to be in the same state for unification.

I would like to unblock the app-config team asap.

@ramya-rao-a
Copy link
Contributor

Ok, sounds good.
I'll assign the retry tasks in core to you @HarshaNalluru as at this point you are most familiar with all the different requirements here :)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
App Configuration Azure.ApplicationModel.Configuration Client This issue points to a problem in the data-plane of the library.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants