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 failed client requests #1562

Merged
merged 3 commits into from Jun 23, 2017

Conversation

Projects
None yet
2 participants
@PtrTeixeira
Contributor

PtrTeixeira commented Jun 8, 2017

In situations when the client fails with certain error codes,
it should be able to retry the request. Ideally, it would
retry the request to a different host so that in the event that
an instance malfunctioned or was removed, the request wasn't
consistently failing.

/cc @ssalinas

Retry failed client requests
In situations when the client fails with certain error codes,
it should be able to retry the request. Ideally, it would
retry the request to a different host so that in the event that
an instance malfunctioned or was removed, the request wasn't
consistently failing.
@ssalinas

This comment has been minimized.

Show comment
Hide comment
@ssalinas

ssalinas Jun 8, 2017

Member

The guava retrying looks pretty straightforward. Some things to keep in mind:

  • the HttpClient generally also has its own retries built in as well. We should make sure to keep that in mind when configuring how many times/how often to retry.
  • getHost is currently a random selection. To make this particular case more useful, we could probably update it to exclude a host that has already been tried in the case that a previous host has failed.
  • retry attempts and strategy should be configurable. We can put the methods to set them and add defaults in the SingularityClientProvider class
Member

ssalinas commented Jun 8, 2017

The guava retrying looks pretty straightforward. Some things to keep in mind:

  • the HttpClient generally also has its own retries built in as well. We should make sure to keep that in mind when configuring how many times/how often to retry.
  • getHost is currently a random selection. To make this particular case more useful, we could probably update it to exclude a host that has already been tried in the case that a previous host has failed.
  • retry attempts and strategy should be configurable. We can put the methods to set them and add defaults in the SingularityClientProvider class
Respond to PR comments
In particular, make retry more configurable by allowing the user to set
the number of attempts and the strategy to determine whether a response
should be retried. Also explicitly disabled retries on the request, so
that it would be entirely handled within the Guava retryer. The reason
that retries within the HttpClient can't be re-used is that doing so
would just retry the given HttpRequest, and this needs to be able to
alter the HttpRequest between attempts. Also is a little more careful
about retrying to different hosts on requests.

@PtrTeixeira PtrTeixeira requested a review from ssalinas Jun 15, 2017

@ssalinas

Overall this looks good to me. Not sure of the best way to test it out, but we can merge it to hs_staging branches for some internal services to try

Don't re-run url builder
I was doing something that looked like
```
url = hostToUrl(host)
actualUrl = hostToUrl(url)
```
which was very obviously wrong. This fixes it so that
it only builds the URL once, which is the correct
write way to do this.

@PtrTeixeira PtrTeixeira added the hs_qa label Jun 21, 2017

@ssalinas ssalinas modified the milestone: 0.16.0 Jun 23, 2017

@ssalinas ssalinas merged commit dce0c86 into master Jun 23, 2017

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@ssalinas ssalinas deleted the retry-client-requests branch Jun 23, 2017

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