Allows configuration of status codes that cause Ribbon retries (OpenFeign#492)

JonathanO · adriancole · commit 05c5f9264603 · 2017-05-06T13:03:32.000+08:00
diff --git a/ribbon/src/main/java/feign/ribbon/LBClient.java b/ribbon/src/main/java/feign/ribbon/LBClient.java
@@ -20,7 +20,6 @@
 import com.netflix.client.ClientRequest;
 import com.netflix.client.IResponse;
 import com.netflix.client.RequestSpecificRetryHandler;
-import com.netflix.client.RetryHandler;
 import com.netflix.client.config.CommonClientConfigKey;
 import com.netflix.client.config.IClientConfig;
 import com.netflix.loadbalancer.ILoadBalancer;
@@ -29,12 +28,14 @@
 import java.net.URI;
 import java.util.Arrays;
 import java.util.Collection;
+import java.util.Collections;
 import java.util.LinkedHashMap;
+import java.util.LinkedHashSet;
 import java.util.Map;
+import java.util.Set;
 
 import feign.Client;
 import feign.Request;
-import feign.RequestTemplate;
 import feign.Response;
 import feign.Util;
 
@@ -44,21 +45,34 @@ public final class LBClient extends
   private final int connectTimeout;
   private final int readTimeout;
   private final IClientConfig clientConfig;
+  private final Set<Integer> retryableStatusCodes;
 
   public static LBClient create(ILoadBalancer lb, IClientConfig clientConfig) {
     return new LBClient(lb, clientConfig);
   }
 
+  static Set<Integer> parseStatusCodes(String statusCodesString) {
+    if (statusCodesString == null || statusCodesString.isEmpty()) {
+      return Collections.emptySet();
+    }
+    Set<Integer> codes = new LinkedHashSet<Integer>();
+    for (String codeString: statusCodesString.split(",")) {
+      codes.add(Integer.parseInt(codeString));
+    }
+    return codes;
+  }
+
   LBClient(ILoadBalancer lb, IClientConfig clientConfig) {
     super(lb, clientConfig);
     this.clientConfig = clientConfig;
     connectTimeout = clientConfig.get(CommonClientConfigKey.ConnectTimeout);
     readTimeout = clientConfig.get(CommonClientConfigKey.ReadTimeout);
+    retryableStatusCodes = parseStatusCodes(clientConfig.get(LBClientFactory.RetryableStatusCodes));
   }
 
   @Override
   public RibbonResponse execute(RibbonRequest request, IClientConfig configOverride)
-      throws IOException {
+          throws IOException, ClientException {
     Request.Options options;
     if (configOverride != null) {
       options =
@@ -69,6 +83,10 @@ public RibbonResponse execute(RibbonRequest request, IClientConfig configOverrid
       options = new Request.Options(connectTimeout, readTimeout);
     }
     Response response = request.client().execute(request.toRequest(), options);
+    if (retryableStatusCodes.contains(response.status())) {
+      response.close();
+      throw new ClientException(ClientException.ErrorType.SERVER_THROTTLED);
+    }
     return new RibbonResponse(request.getUri(), response);
   }
 
diff --git a/ribbon/src/main/java/feign/ribbon/LBClientFactory.java b/ribbon/src/main/java/feign/ribbon/LBClientFactory.java
@@ -1,8 +1,10 @@
 package feign.ribbon;
 
 import com.netflix.client.ClientFactory;
+import com.netflix.client.config.CommonClientConfigKey;
 import com.netflix.client.config.DefaultClientConfigImpl;
 import com.netflix.client.config.IClientConfig;
+import com.netflix.client.config.IClientConfigKey;
 import com.netflix.loadbalancer.ILoadBalancer;
 
 public interface LBClientFactory {
@@ -21,10 +23,18 @@ public LBClient create(String clientName) {
     }
   }
 
+  IClientConfigKey<String> RetryableStatusCodes = new CommonClientConfigKey<String>("RetryableStatusCodes") {};
+
   final class DisableAutoRetriesByDefaultClientConfig extends DefaultClientConfigImpl {
     @Override
     public int getDefaultMaxAutoRetriesNextServer() {
       return 0;
     }
+
+    @Override
+    public void loadDefaultValues() {
+      super.loadDefaultValues();
+      putDefaultStringProperty(LBClientFactory.RetryableStatusCodes, "");
+    }
   }
 }
diff --git a/ribbon/src/test/java/feign/ribbon/LBClientTest.java b/ribbon/src/test/java/feign/ribbon/LBClientTest.java
@@ -1,7 +1,5 @@
 package feign.ribbon;
 
-import static org.assertj.core.api.Assertions.assertThat;
-
 import java.net.URI;
 import java.net.URISyntaxException;
 import java.nio.charset.Charset;
@@ -14,8 +12,18 @@
 import feign.Request;
 import feign.ribbon.LBClient.RibbonRequest;
 
+import static org.assertj.core.api.Assertions.assertThat;
+
 public class LBClientTest {
 
+  @Test
+  public void testParseCodes() {
+    assertThat(LBClient.parseStatusCodes("")).isEmpty();
+    assertThat(LBClient.parseStatusCodes(null)).isEmpty();
+    assertThat(LBClient.parseStatusCodes("504")).contains(504);
+    assertThat(LBClient.parseStatusCodes("503,504")).contains(503, 504);
+  }
+
   @Test
   public void testRibbonRequest() throws URISyntaxException {
     // test for RibbonRequest.toRequest()
diff --git a/ribbon/src/test/java/feign/ribbon/RibbonClientTest.java b/ribbon/src/test/java/feign/ribbon/RibbonClientTest.java
@@ -255,6 +255,30 @@ public void ioExceptionRetryWithBuilder() throws IOException, InterruptedExcepti
     // TODO: verify ribbon stats match
     // assertEquals(target.lb().getLoadBalancerStats().getSingleServerStat())
   }
+
+  @Test
+  public void ribbonRetryOnStatusCodes() throws IOException, InterruptedException {
+    server1.enqueue(new MockResponse().setResponseCode(502));
+    server2.enqueue(new MockResponse().setResponseCode(503));
+
+    getConfigInstance().setProperty(serverListKey(), hostAndPort(server1.url("").url()) + "," + hostAndPort(server2.url("").url()));
+    getConfigInstance().setProperty(client() + ".ribbon.MaxAutoRetriesNextServer", 1);
+    getConfigInstance().setProperty(client() + ".ribbon.RetryableStatusCodes", "503,502");
+
+    TestInterface
+            api =
+            Feign.builder().client(RibbonClient.create()).retryer(Retryer.NEVER_RETRY)
+                    .target(TestInterface.class, "http://" + client());
+
+    try {
+      api.post();
+      fail("No exception thrown");
+    } catch (Exception ignored) {
+
+    }
+    assertEquals(1, server1.getRequestCount());
+    assertEquals(1, server2.getRequestCount());
+  }
   
   @Test
   public void testFeignOptionsClientConfig() {

Original file line number	Diff line number	Diff line change
`@@ -1,8 +1,10 @@`
`1`	`1`	`package feign.ribbon;`
`2`	`2`
`3`	`3`	`import com.netflix.client.ClientFactory;`
	`4`	`+import com.netflix.client.config.CommonClientConfigKey;`
`4`	`5`	`import com.netflix.client.config.DefaultClientConfigImpl;`
`5`	`6`	`import com.netflix.client.config.IClientConfig;`
	`7`	`+import com.netflix.client.config.IClientConfigKey;`
`6`	`8`	`import com.netflix.loadbalancer.ILoadBalancer;`
`7`	`9`
`8`	`10`	`public interface LBClientFactory {`
`@@ -21,10 +23,18 @@ public LBClient create(String clientName) {`
`21`	`23`	`}`
`22`	`24`	`}`
`23`	`25`
	`26`	`+ IClientConfigKey<String> RetryableStatusCodes = new CommonClientConfigKey<String>("RetryableStatusCodes") {};`
	`27`	`+`
`24`	`28`	`final class DisableAutoRetriesByDefaultClientConfig extends DefaultClientConfigImpl {`
`25`	`29`	`@Override`
`26`	`30`	`public int getDefaultMaxAutoRetriesNextServer() {`
`27`	`31`	`return 0;`
`28`	`32`	`}`
	`33`	`+`
	`34`	`+ @Override`
	`35`	`+ public void loadDefaultValues() {`
	`36`	`+ super.loadDefaultValues();`
	`37`	`+ putDefaultStringProperty(LBClientFactory.RetryableStatusCodes, "");`
	`38`	`+ }`
`29`	`39`	`}`
`30`	`40`	`}`