Allow configuration of status codes that cause Ribbon retries.#492
Allow configuration of status codes that cause Ribbon retries.#492codefromthecrypt merged 6 commits intoOpenFeign:masterfrom transferwise:retry-status-codes
Conversation
codefromthecrypt
left a comment
There was a problem hiding this comment.
some nits but otherwise looks good codewise.
@ryanjbaxter do you agree with the behavior change?
| if (statusCodesString == null || statusCodesString.isEmpty()) { | ||
| return Collections.emptySet(); | ||
| } | ||
| Set<Integer> codes = new HashSet<Integer>(); |
There was a problem hiding this comment.
nit always linkedhashset (prevents distraction in tests etc)
| return new LBClient(lb, clientConfig); | ||
| } | ||
|
|
||
| private Set<Integer> parseStatusCodes(String statusCodesString) { |
There was a problem hiding this comment.
static? also package protected makes this testable
| /** | ||
| * Uses {@link ClientFactory} static factories from ribbon to create an LBClient. | ||
| */ | ||
| public static final class Default implements LBClientFactory { |
There was a problem hiding this comment.
this is a noop as defaults to public static as member type of an interface?
| } | ||
| } | ||
|
|
||
| IClientConfigKey<String> ServerThrottledStatusCodes = new CommonClientConfigKey<String>("ServerThrottledStatusCodes") {}; |
There was a problem hiding this comment.
strange casing for constant..
There was a problem hiding this comment.
Yes, though consistent with the similar statics in CommonClientConfigKey itself, so I thought their might be some weird convention about CommonClientConfigKey instances. If not, I'll change it to something more conventional.
|
Yes I do. I was actually looking at an issue yesterday in Spring Cloud Netflix that was similar, in Cloud Foundry the router will return a |
|
anything on @ryanjbaxter's last comment? in favor or against switching ServerThrottledStatusCodes to retryable ones? |
|
FYI here is the change we make in Spring Cloud Netflix. spring-cloud/spring-cloud-netflix#1836 |
|
note: release in a couple days, so here's a chance! |
|
Hi, I've done the rename. |
This reintroduces a behaviour present in the original Ribbon RestClient which allowed a 503 status code to be treated as a connection failure by Ribbon's retry mechanisms. Further, it makes it configurable, allowing you to specify which status codes are considered to be server throttled.
By default it preserves the current Feign ribbon-client behaviour.
I've added this feature since in our environment there's often a gateway in front of an application server which will return 502 or 503 if the application server instance is unavailable. These should be treated as retryable (but only on another server) as per the original RestClient behaviour.