Skip to content

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.

5 participants