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.

Sign up for GitHub

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

Jump to bottom

Retry failed client requests #1562

Merged
merged 3 commits into from
Jun 23, 2017
Merged

Retry failed client requests #1562

merged 3 commits into from
Jun 23, 2017

Conversation

PtrTeixeira
Copy link
Contributor

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

@PtrTeixeira
Retry failed client requests
08dfeeb 
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
Copy link
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

@PtrTeixeira
Respond to PR comments
57fe03b 
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 June 15, 2017 19:36
ssalinas
ssalinas reviewed Jun 20, 2017
View reviewed changes
Copy link
Member

@ssalinas ssalinas left a comment

Choose a reason for hiding this comment

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

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

@PtrTeixeira
Don't re-run url builder
bdd1008 
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.
@ssalinas ssalinas modified the milestone: 0.16.0 Jun 23, 2017
@ssalinas ssalinas merged commit dce0c86 into master Jun 23, 2017
@ssalinas ssalinas deleted the retry-client-requests branch June 23, 2017 18:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ssalinas ssalinas ssalinas left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
0.16.0
Development

Successfully merging this pull request may close these issues.
2 participants
@PtrTeixeira @ssalinas