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

[HttpClient] Parameterize list of retryable methods #38426

Merged
merged 1 commit into from
Oct 20, 2020

Conversation

jderusse
Copy link
Member

@jderusse jderusse commented Oct 6, 2020

Q A
Branch? master
Bug fix? no
New feature? yes
Deprecations? no
Tickets /
License MIT
Doc PR TODO

Retrying non-idempotent methods is not always acceptable for user. This PR adds an easy way to configure this behavior.

The RetryDeciderInterface::shouldRetry() now take the exception in parameter, in order to let decider not retrying the request when the methods should never by retried.

With #38420, this code would belongs to the RetryStrategy implementation, and would return an NeverRetryDecider when method is not allowed.

@jderusse
Copy link
Member Author

jderusse commented Oct 6, 2020

I wonder if the implementation/configuration shouldn't be more complicated:

  • 423, 425, 429, 503 => always
  • 500, 502, 504, 507, 510 => idempotent only

Maybe:
ChainDecider

  • StatusCodeDecider([423, 425, 429, 503])
  • IdempotentStatusCodeDecider([500, 502, 504, 507, 510], [GET, HEAD, PUT, DELETE, TRACE, OPTIONS, CONNECT]

@jderusse jderusse force-pushed the retry-method branch 3 times, most recently from 68a37b1 to 070c1be Compare October 6, 2020 15:16
@nicolas-grekas nicolas-grekas added this to the 5.2 milestone Oct 6, 2020
@jderusse jderusse changed the base branch from master to 5.x October 6, 2020 21:47
@jderusse jderusse force-pushed the retry-method branch 2 times, most recently from 2d35c26 to e40e360 Compare October 6, 2020 21:48
@jderusse
Copy link
Member Author

jderusse commented Oct 6, 2020

failling test are related to Could not parse version constraint >=5.x: Invalid version string "5.x" And related to async-aws that should be fixed by async-aws/aws#802

@jderusse jderusse force-pushed the retry-method branch 2 times, most recently from f6270bb to 73db919 Compare October 7, 2020 10:59
Copy link
Member

@Nyholm Nyholm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you. I like this. I added a bunch of questions for things I dont understand.

@jderusse jderusse force-pushed the retry-method branch 5 times, most recently from 3f9fb7a to a7f7ec5 Compare October 8, 2020 11:11
@nicolas-grekas
Copy link
Member

Let's settle on #38532 before merging (and rebasing) this one.

Copy link
Member

@nicolas-grekas nicolas-grekas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for exceptions, we could use the status code 0

@jderusse jderusse force-pushed the retry-method branch 2 times, most recently from 2c1ee99 to 83d8d10 Compare October 16, 2020 16:53
@fabpot
Copy link
Member

fabpot commented Oct 20, 2020

Thank you @jderusse.

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

Successfully merging this pull request may close these issues.

5 participants