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

Retry Plugin should also retry responses with status 5xx #126

Closed
dbu opened this issue Dec 4, 2018 · 3 comments · Fixed by #139
Closed

Retry Plugin should also retry responses with status 5xx #126

dbu opened this issue Dec 4, 2018 · 3 comments · Fixed by #139
Assignees
Milestone

Comments

@dbu
Copy link
Contributor

dbu commented Dec 4, 2018

Q A
Bug? no
New Feature? yes
Version 2

Actual Behavior

We only retry if there is an exception thrown.

Expected Behavior

We should also retry when a response arrives but the status is indicating a server error.

Possible Solutions

  • a) Have an exception decider and a response decider (and same for delay)
  • b) Expand the decider and delay functions to also pass them the response. This would be a hard BC break
@dbu dbu added this to the v2.0.0 milestone Dec 4, 2018
@dbu
Copy link
Contributor Author

dbu commented Dec 4, 2018

i think i prefer a) to avoid the BC break. also, there will never be both exception and response and the retry plugin knows in which part it is.

i suggest keeping the names for the exception decider and delay and adding a response_decider and response_error_delay that have request and response as argument, and no exception.

@joelwurtz
Copy link
Member

Prefer a) also

But would like to change the name and deprecate older ones for consistency

@dbu
Copy link
Contributor Author

dbu commented Dec 25, 2018

as we do version 2, i will change the name and hint with a comment in the code that the name changed (in addition to the changelog)

@dbu dbu self-assigned this Dec 29, 2018
@dbu dbu mentioned this issue Dec 29, 2018
2 tasks
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 a pull request may close this issue.

2 participants