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

Extending list of retryable codes #786

Merged

Conversation

SergeyMenshykh
Copy link
Member

Motivation, Context and Description

This change is required to improve SK SDK resilience while working with REST API by extending list of HTTP status codes DefaultHttpRetryHandler retries on.

Contribution Checklist

@github-actions github-actions bot added .NET Issue or Pull requests regarding .NET code kernel.core labels May 3, 2023
@SergeyMenshykh SergeyMenshykh added PR: ready for review All feedback addressed, ready for reviews and removed .NET Issue or Pull requests regarding .NET code kernel.core labels May 3, 2023
@github-actions github-actions bot added .NET Issue or Pull requests regarding .NET code kernel.core labels May 3, 2023
@adrianwyatt adrianwyatt self-requested a review May 4, 2023 00:19
@adrianwyatt adrianwyatt added PR: feedback to address Waiting for PR owner to address comments/questions and removed PR: ready for review All feedback addressed, ready for reviews labels May 4, 2023
@SergeyMenshykh SergeyMenshykh added PR: ready for review All feedback addressed, ready for reviews and removed PR: feedback to address Waiting for PR owner to address comments/questions labels May 4, 2023
@RogerBarreto
Copy link
Member

Should we consider:

425(Too Early)?
507 InsufficientStorage?

@adrianwyatt adrianwyatt enabled auto-merge (squash) May 4, 2023 15:37
@adrianwyatt adrianwyatt merged commit bc399e4 into microsoft:main May 4, 2023
@SergeyMenshykh
Copy link
Member Author

Should we consider:

425(Too Early)? 507 InsufficientStorage?

Not sure about 425 one because it's related to a specific scenario when a client wants to send data in advance of the secure connection being set up to optimize the time required for TLS to create a secure connection.

There're no clear guidelines about the 507 one. Some sources say that it's a temporary issue implying that it can be retried the other ones say that it should not be retried in a scope of the same action?

Taking into account that it's always possible to add any additional status code when configuring Kernel, I would not add these codes now, and wait until they are requested be a default ones.

@adrianwyatt
Copy link
Contributor

Should we consider:

425(Too Early)? 507 InsufficientStorage?

Good callout - we'll use this PR to align with MS practices, but I would welcome an issue for 425/507

codebrain pushed a commit to searchpioneer/semantic-kernel that referenced this pull request May 16, 2023
### Motivation, Context and Description

This change is required to improve SK SDK resilience while working with
REST API by extending list of HTTP status codes DefaultHttpRetryHandler
retries on.
dehoward pushed a commit to lemillermicrosoft/semantic-kernel that referenced this pull request Jun 1, 2023
### Motivation, Context and Description

This change is required to improve SK SDK resilience while working with
REST API by extending list of HTTP status codes DefaultHttpRetryHandler
retries on.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
.NET Issue or Pull requests regarding .NET code PR: ready for review All feedback addressed, ready for reviews
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants