-
Notifications
You must be signed in to change notification settings - Fork 498
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
Add 429 retry and logging in BundleHandler #2400
Conversation
@@ -391,6 +391,25 @@ private async Task<EntryComponent> ExecuteRequests(Hl7.Fhir.Model.Bundle respons | |||
|
|||
await request.Handler.Invoke(httpContext); | |||
|
|||
// we will retry a 429 one time per request in the bundle | |||
if (httpContext.Response.StatusCode == (int)HttpStatusCode.TooManyRequests | |||
&& _bundleType == BundleType.Batch) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we care if it is a Batch request? Couldn't we do the same thing for a Transaction request?
var retryAfterValues = httpContext.Response.Headers.GetCommaSeparatedValues("Retry-After"); | ||
if (retryAfterValues != StringValues.Empty) | ||
{ | ||
if (int.TryParse(retryAfterValues[0], out var retryHeaderValue)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could this be collapsed into the if statement above it so we only need one?
@@ -286,7 +286,7 @@ public async Task GivenABundle_WhenProcessed_CertainResponseHeadersArePropagated | |||
} | |||
|
|||
[Fact] | |||
public async Task GivenABundle_WhenOneRequestProducesA429_SubsequentRequestAreSkipped() | |||
public async Task GivenABundle_WhenOneRequestProducesA429_429IsRetriedThenSubsequentRequestAreSkipped() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it worth adding a test that has a retry succeed to show the happy retry path?
{ | ||
retryDelay = retryHeaderValue; | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it worth to retry in absence of "Retry-After" header?
Feels that delay 2 milliseconds wouldn't do much to solve problem...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was worried that 2 seconds might be too long :). I used 2 seconds as a fall back, because we have similar logic in the CosmosFhirStore code. I would still like to try, given that we did receive a 429.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://docs.microsoft.com/en-us/dotnet/api/system.threading.tasks.task.delay?view=net-6.0
reates a cancellable task that completes after a specified number of milliseconds.
It's not a seconds...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank for you for catching it Ivan! Sorry I missed the "milliseconds" in your previous comment. I merged this too quickly! I will put up a new PR to fix it.
Description
We sometimes encounter 429 errors when processing a bundle. If we receive a 429 at the BundleHandler layer, we abort processing of the bundle and skip the remaining resources. Here we add an additional retry (in addition to the retry present in the data store layer) that will execute one time per resource which encounters a 429. Also some logging is added to help us identify this occurrence.
Related issues
Addresses [issue AB#87196].
Testing
Unit was updated.
FHIR Team Checklist
Semver Change (docs)
Patch|Skip|Feature|Breaking (reason)