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.

Sign up for GitHub

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

Jump to bottom

[app-configuration] Handle throttling retry headers properly #6431

Merged
merged 13 commits into from
Dec 6, 2019
Merged

[app-configuration] Handle throttling retry headers properly #6431

merged 13 commits into from
Dec 6, 2019

Conversation

richardpark-msft
Copy link
Member

This PR adds in retry-after-ms and x-ms-retry-after-ms to the list
of headers we check alongside Retry-After, bringing it to parity with
what the C# SDK does.

Fixes #6408

@richardpark-msft
Check multiple headers in ThrottlingRetryPolicy
ff6d39e 
This PR adds in `retry-after-ms` and `x-ms-retry-after-ms` to the list
of headers we check alongside `Retry-After`, bringing it to parity with
what the C# SDK does.

Fixes Azure#6408
@richardpark-msft
Copy link
Member Author

I need to look into this more. The code is correct but it won't quite solve the issue in AppConfig.

@richardpark-msft
The throttling policy didn't properly "retry" (the underlying seriali…
21b5873 
…ation layer throws on 429, so the throttling policy never gets a chance to handle it).

One weakness here that I'd like to discuss - I believe I'd need to add some retry limitations here, even if they just default.
@richardpark-msft
Copy link
Member Author

Reviewers: I believe the right thing here is to make the throttlingRetryPolicy take some new parameters to control the retry count. It doesn't today which makes me think I'm missing something.

Can people with more experience with the design of core-http help me understand if I'm just missing the point.

richardpark-msft
richardpark-msft commented Dec 6, 2019
View reviewed changes
Copy link
Member Author

@richardpark-msft richardpark-msft left a comment

Choose a reason for hiding this comment

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

Adding some comments for reviewers.

sdk/core/core-http/lib/restError.ts Outdated Show resolved Hide resolved
@richardpark-msft
Copy link
Member Author

Last update - moved my policy changes to a version in app-configuration rather than being in core-http.

@richardpark-msft richardpark-msft changed the title [core-http] Check multiple headers in ThrottlingRetryPolicy [app-configuration] Handle throttling retry headers properly Dec 6, 2019
@chradek
Copy link
Contributor

chradek commented Dec 6, 2019

Can you run the live tests before merging as a sanity check?

@richardpark-msft richardpark-msft merged commit 670e4b7 into Azure:master Dec 6, 2019
@ramya-rao-a
Copy link
Contributor

cc @bterlson, @daviwil and @xirzec as this PR is dealing with pipeline policies

@richardpark-msft
Copy link
Member Author

Can you run the live tests before merging as a sanity check?

Always do (and did!)

@richardpark-msft
Copy link
Member Author

cc @bterlson, @daviwil and @xirzec as this PR is dealing with pipeline policies

I ended up moving the policy to be just in app-configuration but I'm interested in how we could make this more general. I ran into a few issues and it made me wonder how the pieces for retry are intended to fit together.

@bterlson
Copy link
Member

bterlson commented Dec 7, 2019

Why not use the throttling policy in core-http? Sorry I may have missed much context.

@richardpark-msft
Copy link
Member Author

Why not use the throttling policy in core-http? Sorry I may have missed much context.

The throttling policy in core-http had a few issues (none of which are insurmountable but I was very unsure of the intention behind them):

  • It assumes the response will come back and it can handle it normally. AppConfig throws a RestError for 429 so the delay wasn't happening. Wasn't sure if this was a difference in how appconfig works or if this is the way it works for everyone and the policy was just incorrect.
  • It only handles the Retry-After header. There are at least two others that apparently exist (dragged them in from the C# implement)

I'd love to revisit how we can make this universal (wasn't super happy about moving it out but I didn't see a feasible way to test that I'd done it correctly and not broken others).

@richardpark-msft richardpark-msft deleted the richardpark-ac-429 branch December 7, 2019 00:28
@bterlson
Copy link
Member

bterlson commented Dec 7, 2019

Agree, we should pursue unification here. /cc @daviwil I think you have throttlingRetryPolicy chilling out in the default pipeline anyway, so it'd be nice not to have two of them.

Supporting more headers seems non-breaking and relatively safe to do in core-http.

I'm less sure about retrying on errors, but my feeling is that whether or not we've deserialized the response body into an error, if the response is a 429 it's safe to engage the default throttling policy. @johanste you may have some thoughts on this, care to share?

@richardpark-msft richardpark-msft mentioned this pull request Dec 10, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chradek chradek chradek approved these changes

@daviwil daviwil Awaiting requested review from daviwil

@ramya-rao-a ramya-rao-a Awaiting requested review from ramya-rao-a

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

AppConfigurationClient does not retry when a request is throttled
4 participants
@richardpark-msft @chradek @ramya-rao-a @bterlson