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

Make retry backoff and status codes customizable #421

Merged
merged 1 commit into from
Jul 13, 2023

Conversation

mmmeeedddsss
Copy link
Contributor

RequestsHTTPTransport does not allow customization of urllib3's retry method.

In this PR, I added two new parameters to RequestsHTTPTransport for supporting most of the possible customization cases when the RequestsHTTPTransport is used with retries: retry_backoff_factor & retry_status_forcelist. (See urllib3's doc for Retry)

Two notes on compatibility:

  • I wanted to make this change backwards compatible as much as it could be. For the clients of gql that use RequestsHTTPTransport without keyed args, this change is backward incompatible since it changes the order of argument method , but I can refactor to move two new parameters to the place after method. In that case, the argument retries and the new two arguments related to retries will be separated, which is not a huge deal.
  • I believe most elegant solution for the customizability here would accept either an HTTPAdapter object or a Retry object from outside, but I didn't want to break the compatibility in such a huge way.

@codecov
Copy link

codecov bot commented Jul 12, 2023

Codecov Report

Patch coverage: 100.00% and no project coverage change.

Comparison is base (8b52134) 100.00% compared to head (8723d45) 100.00%.

Additional details and impacted files
@@            Coverage Diff            @@
##            master      #421   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           27        27           
  Lines         2385      2388    +3     
=========================================
+ Hits          2385      2388    +3     
Impacted Files Coverage Δ
gql/transport/requests.py 100.00% <100.00%> (ø)

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@leszekhanusz
Copy link
Collaborator

Thanks for the PR!
You should be able to fix the lint issues by running make check as described in the CONTRIBUTING.md file.

@mmmeeedddsss
Copy link
Contributor Author

@leszekhanusz Now it should be okay, could you re-run the tests?

@leszekhanusz
Copy link
Collaborator

* I wanted to make this change backwards compatible as much as it could be. For the clients of gql that use `RequestsHTTPTransport` without keyed args, this change is backward incompatible since it changes the order of argument `method `, but I can refactor to move two new parameters to the place after `method`. In that case, the argument `retries ` and the new two arguments related to retries will be separated, which is not a huge deal.

Yes, If you could please move the new arguments to the end that would be best.
The probability of a breaking chance seems quite low in this case but you never know.

* I believe most elegant solution for the customizability here would accept either an HTTPAdapter object or a Retry object from outside, but I didn't want to break the compatibility in such a huge way.

Good, that's fine like this.

@mmmeeedddsss
Copy link
Contributor Author

@leszekhanusz Made the change, thank you for the guidance. It should be fully backward compatible now

@leszekhanusz leszekhanusz merged commit f35970f into graphql-python:master Jul 13, 2023
15 checks passed
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

2 participants