Skip to content

adjust decider to not retry client errors #125

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

Merged
merged 1 commit into from
Dec 9, 2018
Merged

Conversation

dbu
Copy link
Contributor

@dbu dbu commented Dec 2, 2018

Q A
Bug fix? yes
New feature? no
BC breaks? kindof
Deprecations? no
Related tickets fixes #80
Documentation -
License MIT

What's in this PR?

Do not retry if the error is caused by the server returning a HTTP error code indicating that the request was bad.

Why?

There is no point in retrying those.

Checklist

  • Updated CHANGELOG.md to describe BC breaks / deprecations | new feature | bugfix

To Do

@dbu dbu requested a review from Nyholm December 2, 2018 09:45
@dbu dbu force-pushed the do-not-retry-client-errors branch from b12d8ac to 874052a Compare December 2, 2018 17:21
@joelwurtz
Copy link
Member

Not sure why this is needed ? Since only the ErrorPlugin transform to HttpException AFAIK. Also you can configure the ErrorPlugin to only throw when response status code is >= 500.

@dbu
Copy link
Contributor Author

dbu commented Dec 3, 2018

@joelwurtz indeed the response exception is an edge case, the expected exceptions are transfer problems. but i think it still does make sense.

however, what would be more relevant is to look at non exception responses to check them for server side errors and retry those as well.

@Nyholm
Copy link
Member

Nyholm commented Dec 4, 2018

I like this.

I also think it is strange that we only consider exceptions. Why dont we also check the response? Ie this plugin is useless without the ErrorPlugin.

@joelwurtz
Copy link
Member

we should certainly change the decider function to accept a ResponseInterface inside it then

@dbu dbu force-pushed the do-not-retry-client-errors branch from 874052a to eb9ec50 Compare December 4, 2018 10:42
@dbu
Copy link
Contributor Author

dbu commented Dec 4, 2018

i created #126 to discuss handling server error responses in addition to exceptions. please give your input there.

ok if we merge the tweak for exception handling here separately?

@dbu dbu force-pushed the do-not-retry-client-errors branch from eb9ec50 to 66bf4d6 Compare December 4, 2018 10:45
@dbu
Copy link
Contributor Author

dbu commented Dec 8, 2018

this is only about the exception - okay if we merge it?

@Nyholm Nyholm merged commit 18e36cc into 2.x Dec 9, 2018
@Nyholm
Copy link
Member

Nyholm commented Dec 9, 2018

Thank you

@Nyholm Nyholm deleted the do-not-retry-client-errors branch December 9, 2018 11:09
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.

4 participants