Add retry http request filter for transient errors. Integrate filter with ClientConfig. #8977
Conversation
pujagani
force-pushed
the
retry_transient_errors
branch
from
690ad60 to
912b0ec
Compare
|
SonarCloud Quality Gate failed. |
| size = "small", | ||
| srcs = glob(["*.java"]), | ||
| srcs = glob(["*.java"], | ||
| exclude = MEDIUM_TESTS), |
There was a problem hiding this comment.
Unless buildifier reformatted things this way, it's fine to put the exclude on the same line
| e.getAttemptCount())); | ||
|
|
||
| // Retry if server is unavailable or an internal server error occurs without response body. | ||
| private static final RetryPolicy<HttpResponse> serverErrorPolicy = |
There was a problem hiding this comment.
I'd be tempted to abandon hope if the response is an internal error: it's likely sending the request again will cause the same problem.
There was a problem hiding this comment.
Here the retry is for 500 error code without any response payload. In this iteration, the retry is 2 attempts then results in failure if the error still exists. I am thinking it should be okay. What do you suggest?
pujagani
force-pushed
the
retry_transient_errors
branch
from
912b0ec to
a224c3a
Compare
|
SonarCloud Quality Gate failed. |
pujagani
force-pushed
the
retry_transient_errors
branch
from
8358831 to
412445d
Compare
|
SonarCloud Quality Gate failed. |
pujagani
force-pushed
the
retry_transient_errors
branch
from
412445d to
9be85e2
Compare
| // Retry if server is unavailable or an internal server error occurs without response body. | ||
| private static final RetryPolicy<HttpResponse> serverErrorPolicy = | ||
| new RetryPolicy<HttpResponse>() | ||
| .handleResultIf(response -> response.getStatus() == HTTP_INTERNAL_ERROR && |
There was a problem hiding this comment.
The webdriver spec uses the 500 response for error responses, so this might be risky.
There was a problem hiding this comment.
Here the retry is for 500 error code without any response payload. In this iteration, the retry is 2 attempts then results in failure if the error still exists. I am thinking it should be okay. What do you suggest?
There was a problem hiding this comment.
Let's ship it and see what breaks. I can't think of a circumstance when the remote end will send a 500 without a message body.
java/test/org/openqa/selenium/remote/http/RetryRequestTest.java
Outdated
Show resolved
Hide resolved
pujagani
force-pushed
the
retry_transient_errors
branch
from
45c316f to
3d73887
Compare
|
SonarCloud Quality Gate failed. |
| // Retry if server is unavailable or an internal server error occurs without response body. | ||
| private static final RetryPolicy<HttpResponse> serverErrorPolicy = | ||
| new RetryPolicy<HttpResponse>() | ||
| .handleResultIf(response -> response.getStatus() == HTTP_INTERNAL_ERROR && |
There was a problem hiding this comment.
Let's ship it and see what breaks. I can't think of a circumstance when the remote end will send a 500 without a message body.
3 participants
Thanks for contributing to Selenium!
A PR well described will help maintainers to quickly review and merge it
Before submitting your PR, please check our contributing guidelines.
Avoid large PRs, help reviewers by making them as simple and short as possible.
Description
Related to #8167. Add retry mechanism using Failsafe.
Motivation and Context
When a client makes a request to the server, it might not get fulfilled due to different errors. One category of errors is transient in nature. In such cases, the request might be fulfilled if it is reattempted with a delay. The changes introduce a retry mechanism as a filter. The retry policies handler read timeout, connection failures and server errors (server unavailable and internal server error without response body). Rest of the failures are not retried.
Types of changes
Checklist