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

Allow Retry class to determine to retry based on response #2500

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

EnricoMi
Copy link

The Retry class allows library users to implement their own retry logic. The library calls into Retry.is_retry to determine if a request is to be retried based on the status code. Sometimes the status code is not sufficient to identify a retry-able error (#1445). The actual response, which includes the status code, the reason, response headers and payload, among others, is sometimes needed.

Replacing the is_retry method with is_response_retry allows users to get hold of the actual response. This change keeps is_retry to avoid breaking existing code.

@codecov
Copy link

codecov bot commented Dec 11, 2021

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (73953a0) 100.00% compared to head (958642e) 100.00%.
Report is 408 commits behind head on main.

❗ Current head 958642e differs from pull request most recent head 20170af. Consider uploading reports for the commit 20170af to get more accurate results

Additional details and impacted files
@@            Coverage Diff            @@
##              main     #2500   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           25        25           
  Lines         2530      2532    +2     
=========================================
+ Hits          2530      2532    +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@EnricoMi EnricoMi changed the title Allow Retry class to determine whether to retry based on response Allow Retry class to determine to retry based on response Dec 11, 2021
@EnricoMi EnricoMi marked this pull request as ready for review December 11, 2021 20:32
@EnricoMi
Copy link
Author

@sethmlarson to be clear: this is not intended for 2.0 but the next 1.x release. So I presume this should target branch 1.26.x, right?

@sethmlarson
Copy link
Member

@EnricoMi We're not adding new features to 1.x as there won't be a 1.27 release. All new features must go into 2.0.

@EnricoMi
Copy link
Author

So no quick fix. And a non-breaking fix is not required. What is the time horizon of the 2.0 release.

@sethmlarson
Copy link
Member

@EnricoMi We don't have a date in mind, we're trying to finish these blocking issues before a release though.

@EnricoMi
Copy link
Author

Should is_retry be marked as deprecated? Or shall both methods co-exist in the future?

@JannKleen
Copy link

@EnricoMi is there any update on this? I need my Retry-class to support a non-standard Retry header and having this merged would make life easier ;)

@ollie-bell
Copy link

Now that 2.0 is in pre-release, is this feature going to be available soon? Thanks!

@EnricoMi EnricoMi force-pushed the branch-retry-with-response branch 2 times, most recently from 958642e to fc5e2b1 Compare January 17, 2023 06:59
@EnricoMi
Copy link
Author

@sethmlarson rebased with latest main. Is this now good to go?

@EnricoMi
Copy link
Author

EnricoMi commented Oct 2, 2023

This is sad...

@JannKleen
Copy link

@sethmlarson as someone interested in using it, is there anything missing in order to get this merged?

@ecerulm
Copy link
Contributor

ecerulm commented Jan 30, 2024

@EnricoMi can you resolve the merge conflict?

@EnricoMi
Copy link
Author

@ecerulm conflicts resolved

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants