From 8d18dfbc3d3f594e4e58e4ecf277624ee5b6e00d Mon Sep 17 00:00:00 2001 From: Ryan Baxter Date: Thu, 11 Jan 2018 10:05:02 -0500 Subject: [PATCH] Set hystrix timeout for Zuul routes based on whether the hystrix command timeout, default timeout, or ribbon timeouts are set. Fixes #2606 (#2633) --- .../route/support/AbstractRibbonCommand.java | 47 ++++++++++++++-- .../HttpClientRibbonCommandFactoryTest.java | 56 +++++++++++++++++++ .../OkHttpRibbonCommandFactoryTest.java | 56 +++++++++++++++++++ ...onCommandCauseFallbackPropagationTest.java | 3 +- 4 files changed, 155 insertions(+), 7 deletions(-) diff --git a/spring-cloud-netflix-core/src/main/java/org/springframework/cloud/netflix/zuul/filters/route/support/AbstractRibbonCommand.java b/spring-cloud-netflix-core/src/main/java/org/springframework/cloud/netflix/zuul/filters/route/support/AbstractRibbonCommand.java index 47d7e929..a21b636a 100644 --- a/spring-cloud-netflix-core/src/main/java/org/springframework/cloud/netflix/zuul/filters/route/support/AbstractRibbonCommand.java +++ b/spring-cloud-netflix-core/src/main/java/org/springframework/cloud/netflix/zuul/filters/route/support/AbstractRibbonCommand.java @@ -17,6 +17,8 @@ package org.springframework.cloud.netflix.zuul.filters.route.support; +import org.apache.commons.logging.Log; +import org.apache.commons.logging.LogFactory; import org.springframework.cloud.netflix.ribbon.RibbonClientConfiguration; import org.springframework.cloud.netflix.ribbon.RibbonHttpResponse; import org.springframework.cloud.netflix.zuul.filters.ZuulProperties; @@ -28,6 +30,7 @@ import org.springframework.http.client.ClientHttpResponse; import com.netflix.client.AbstractLoadBalancerAwareClient; import com.netflix.client.ClientRequest; import com.netflix.client.config.IClientConfig; +import com.netflix.client.config.IClientConfigKey; import com.netflix.client.http.HttpResponse; import com.netflix.config.DynamicIntProperty; import com.netflix.config.DynamicPropertyFactory; @@ -46,6 +49,7 @@ import com.netflix.zuul.context.RequestContext; public abstract class AbstractRibbonCommand, RQ extends ClientRequest, RS extends HttpResponse> extends HystrixCommand implements RibbonCommand { + private static final Log LOGGER = LogFactory.getLog(AbstractRibbonCommand.class); protected final LBC client; protected RibbonCommandContext context; protected ZuulFallbackProvider zuulFallbackProvider; @@ -70,7 +74,7 @@ public abstract class AbstractRibbonCommand 0) { + hystrixTimeout = commandHystrixTimeout; + } + else if( defaultHystrixTimeout > 0) { + hystrixTimeout = defaultHystrixTimeout; + } else { + hystrixTimeout = ribbonTimeout; + } + if(hystrixTimeout < ribbonTimeout) { + LOGGER.warn("The Hystrix timeout of " + hystrixTimeout + "ms for the command " + commandKey + + " is set lower than the combination of the Ribbon read and connect timeout, " + ribbonTimeout + "ms."); + } + return HystrixCommandProperties.Setter().withExecutionIsolationStrategy( + zuulProperties.getRibbonIsolationStrategy()).withExecutionTimeoutInMilliseconds(hystrixTimeout); + } + + @Deprecated + //TODO remove in 2.0.x + protected static Setter getSetter(final String commandKey, ZuulProperties zuulProperties) { + return getSetter(commandKey, zuulProperties, null); + } + protected static Setter getSetter(final String commandKey, - ZuulProperties zuulProperties) { + ZuulProperties zuulProperties, IClientConfig config) { // @formatter:off Setter commandSetter = Setter.withGroupKey(HystrixCommandGroupKey.Factory.asKey("RibbonCommand")) .andCommandKey(HystrixCommandKey.Factory.asKey(commandKey)); - - final HystrixCommandProperties.Setter setter = HystrixCommandProperties.Setter() - .withExecutionIsolationStrategy(zuulProperties.getRibbonIsolationStrategy()).withExecutionTimeoutInMilliseconds( - RibbonClientConfiguration.DEFAULT_CONNECT_TIMEOUT + RibbonClientConfiguration.DEFAULT_READ_TIMEOUT); + final HystrixCommandProperties.Setter setter = createSetter(config, commandKey, zuulProperties); if (zuulProperties.getRibbonIsolationStrategy() == ExecutionIsolationStrategy.SEMAPHORE){ final String name = ZuulConstants.ZUUL_EUREKA + commandKey + ".semaphore.maxSemaphores"; // we want to default to semaphore-isolation since this wraps diff --git a/spring-cloud-netflix-core/src/test/java/org/springframework/cloud/netflix/zuul/filters/route/apache/HttpClientRibbonCommandFactoryTest.java b/spring-cloud-netflix-core/src/test/java/org/springframework/cloud/netflix/zuul/filters/route/apache/HttpClientRibbonCommandFactoryTest.java index 2b95bf38..5ef1fdf7 100644 --- a/spring-cloud-netflix-core/src/test/java/org/springframework/cloud/netflix/zuul/filters/route/apache/HttpClientRibbonCommandFactoryTest.java +++ b/spring-cloud-netflix-core/src/test/java/org/springframework/cloud/netflix/zuul/filters/route/apache/HttpClientRibbonCommandFactoryTest.java @@ -1,6 +1,7 @@ package org.springframework.cloud.netflix.zuul.filters.route.apache; import java.util.HashSet; +import org.junit.After; import org.junit.Before; import org.junit.Test; import org.springframework.cloud.netflix.ribbon.SpringClientFactory; @@ -11,6 +12,9 @@ import org.springframework.cloud.netflix.zuul.filters.route.ZuulFallbackProvider import com.netflix.client.config.DefaultClientConfigImpl; import com.netflix.client.config.IClientConfig; +import com.netflix.client.config.IClientConfigKey; +import com.netflix.config.ConfigurationManager; +import com.netflix.hystrix.strategy.properties.HystrixPropertiesFactory; import static org.junit.Assert.assertEquals; import static org.mockito.Matchers.anyString; @@ -39,6 +43,12 @@ public class HttpClientRibbonCommandFactoryTest { this.ribbonCommandFactory = new HttpClientRibbonCommandFactory(springClientFactory, zuulProperties, new HashSet()); } + @After + public void after() { + ConfigurationManager.getConfigInstance().clear(); + HystrixPropertiesFactory.reset(); + } + @Test public void testHystrixTimeoutValue() throws Exception { RibbonCommandContext context = mock(RibbonCommandContext.class); @@ -47,4 +57,50 @@ public class HttpClientRibbonCommandFactoryTest { assertEquals(2000, ribbonCommand.getProperties().executionTimeoutInMilliseconds().get().intValue()); } + @Test + public void testHystrixTimeoutValueSetting() throws Exception { + ConfigurationManager.getConfigInstance().setProperty("hystrix.command.default.execution.isolation.thread.timeoutInMilliseconds", 50); + RibbonCommandContext context = mock(RibbonCommandContext.class); + doReturn("service").when(context).getServiceId(); + HttpClientRibbonCommand ribbonCommand = this.ribbonCommandFactory.create(context); + assertEquals(50, ribbonCommand.getProperties().executionTimeoutInMilliseconds().get().intValue()); + } + + @Test + public void testHystrixTimeoutValueCommandSetting() throws Exception { + ConfigurationManager.getConfigInstance().setProperty("hystrix.command.service.execution.isolation.thread.timeoutInMilliseconds", 50); + RibbonCommandContext context = mock(RibbonCommandContext.class); + doReturn("service").when(context).getServiceId(); + HttpClientRibbonCommand ribbonCommand = this.ribbonCommandFactory.create(context); + assertEquals(50, ribbonCommand.getProperties().executionTimeoutInMilliseconds().get().intValue()); + } + + @Test + public void testHystrixTimeoutValueCommandAndDefaultSetting() throws Exception { + ConfigurationManager.getConfigInstance().setProperty("hystrix.command.default.execution.isolation.thread.timeoutInMilliseconds", 30); + ConfigurationManager.getConfigInstance().setProperty("hystrix.command.service.execution.isolation.thread.timeoutInMilliseconds", 50); + RibbonCommandContext context = mock(RibbonCommandContext.class); + doReturn("service").when(context).getServiceId(); + HttpClientRibbonCommand ribbonCommand = this.ribbonCommandFactory.create(context); + assertEquals(50, ribbonCommand.getProperties().executionTimeoutInMilliseconds().get().intValue()); + } + + @Test + public void testHystrixTimeoutValueRibbonTimeouts() throws Exception { + SpringClientFactory springClientFactory = mock(SpringClientFactory.class); + ZuulProperties zuulProperties = new ZuulProperties(); + RibbonLoadBalancingHttpClient loadBalancingHttpClient = mock(RibbonLoadBalancingHttpClient.class); + IClientConfig clientConfig = new DefaultClientConfigImpl(); + clientConfig.set(IClientConfigKey.Keys.ConnectTimeout, 100); + clientConfig.set(IClientConfigKey.Keys.ReadTimeout, 500); + doReturn(loadBalancingHttpClient).when(springClientFactory).getClient(anyString(), + eq(RibbonLoadBalancingHttpClient.class)); + doReturn(clientConfig).when(springClientFactory).getClientConfig(anyString()); + HttpClientRibbonCommandFactory ribbonCommandFactory = new HttpClientRibbonCommandFactory(springClientFactory, zuulProperties, new HashSet()); + RibbonCommandContext context = mock(RibbonCommandContext.class); + doReturn("service").when(context).getServiceId(); + HttpClientRibbonCommand ribbonCommand = ribbonCommandFactory.create(context); + assertEquals(600, ribbonCommand.getProperties().executionTimeoutInMilliseconds().get().intValue()); + } + } \ No newline at end of file diff --git a/spring-cloud-netflix-core/src/test/java/org/springframework/cloud/netflix/zuul/filters/route/okhttp/OkHttpRibbonCommandFactoryTest.java b/spring-cloud-netflix-core/src/test/java/org/springframework/cloud/netflix/zuul/filters/route/okhttp/OkHttpRibbonCommandFactoryTest.java index 392a182b..4fcec561 100644 --- a/spring-cloud-netflix-core/src/test/java/org/springframework/cloud/netflix/zuul/filters/route/okhttp/OkHttpRibbonCommandFactoryTest.java +++ b/spring-cloud-netflix-core/src/test/java/org/springframework/cloud/netflix/zuul/filters/route/okhttp/OkHttpRibbonCommandFactoryTest.java @@ -1,6 +1,7 @@ package org.springframework.cloud.netflix.zuul.filters.route.okhttp; import java.util.HashSet; +import org.junit.After; import org.junit.Before; import org.junit.Test; import org.springframework.cloud.netflix.ribbon.SpringClientFactory; @@ -11,6 +12,9 @@ import org.springframework.cloud.netflix.zuul.filters.route.ZuulFallbackProvider import com.netflix.client.config.DefaultClientConfigImpl; import com.netflix.client.config.IClientConfig; +import com.netflix.client.config.IClientConfigKey; +import com.netflix.config.ConfigurationManager; +import com.netflix.hystrix.strategy.properties.HystrixPropertiesFactory; import static org.junit.Assert.assertEquals; import static org.mockito.Matchers.anyString; @@ -39,6 +43,12 @@ public class OkHttpRibbonCommandFactoryTest { commandFactory = new OkHttpRibbonCommandFactory(springClientFactory, zuulProperties, new HashSet()); } + @After + public void after() { + ConfigurationManager.getConfigInstance().clear(); + HystrixPropertiesFactory.reset(); + } + @Test public void testHystrixTimeoutValue() throws Exception { RibbonCommandContext context = mock(RibbonCommandContext.class); @@ -47,4 +57,50 @@ public class OkHttpRibbonCommandFactoryTest { assertEquals(2000, ribbonCommand.getProperties().executionTimeoutInMilliseconds().get().intValue()); } + @Test + public void testHystrixTimeoutValueSetting() throws Exception { + ConfigurationManager.getConfigInstance().setProperty("hystrix.command.default.execution.isolation.thread.timeoutInMilliseconds", 50); + RibbonCommandContext context = mock(RibbonCommandContext.class); + doReturn("service").when(context).getServiceId(); + OkHttpRibbonCommand ribbonCommand = this.commandFactory.create(context); + assertEquals(50, ribbonCommand.getProperties().executionTimeoutInMilliseconds().get().intValue()); + } + + @Test + public void testHystrixTimeoutValueCommandSetting() throws Exception { + ConfigurationManager.getConfigInstance().setProperty("hystrix.command.service.execution.isolation.thread.timeoutInMilliseconds", 50); + RibbonCommandContext context = mock(RibbonCommandContext.class); + doReturn("service").when(context).getServiceId(); + OkHttpRibbonCommand ribbonCommand = this.commandFactory.create(context); + assertEquals(50, ribbonCommand.getProperties().executionTimeoutInMilliseconds().get().intValue()); + } + + @Test + public void testHystrixTimeoutValueCommandAndDefaultSetting() throws Exception { + ConfigurationManager.getConfigInstance().setProperty("hystrix.command.default.execution.isolation.thread.timeoutInMilliseconds", 30); + ConfigurationManager.getConfigInstance().setProperty("hystrix.command.service.execution.isolation.thread.timeoutInMilliseconds", 50); + RibbonCommandContext context = mock(RibbonCommandContext.class); + doReturn("service").when(context).getServiceId(); + OkHttpRibbonCommand ribbonCommand = this.commandFactory.create(context); + assertEquals(50, ribbonCommand.getProperties().executionTimeoutInMilliseconds().get().intValue()); + } + + @Test + public void testHystrixTimeoutValueRibbonTimeouts() throws Exception { + SpringClientFactory springClientFactory = mock(SpringClientFactory.class); + ZuulProperties zuulProperties = new ZuulProperties(); + OkHttpLoadBalancingClient loadBalancingHttpClient = mock(OkHttpLoadBalancingClient.class); + IClientConfig clientConfig = new DefaultClientConfigImpl(); + clientConfig.set(IClientConfigKey.Keys.ConnectTimeout, 100); + clientConfig.set(IClientConfigKey.Keys.ReadTimeout, 500); + doReturn(loadBalancingHttpClient).when(springClientFactory).getClient(anyString(), + eq(OkHttpLoadBalancingClient.class)); + doReturn(clientConfig).when(springClientFactory).getClientConfig(anyString()); + OkHttpRibbonCommandFactory commandFactory = new OkHttpRibbonCommandFactory(springClientFactory, zuulProperties, new HashSet()); + RibbonCommandContext context = mock(RibbonCommandContext.class); + doReturn("service").when(context).getServiceId(); + OkHttpRibbonCommand ribbonCommand = commandFactory.create(context); + assertEquals(600, ribbonCommand.getProperties().executionTimeoutInMilliseconds().get().intValue()); + } + } \ No newline at end of file diff --git a/spring-cloud-netflix-core/src/test/java/org/springframework/cloud/netflix/zuul/filters/route/support/RibbonCommandCauseFallbackPropagationTest.java b/spring-cloud-netflix-core/src/test/java/org/springframework/cloud/netflix/zuul/filters/route/support/RibbonCommandCauseFallbackPropagationTest.java index f7d62131..d70fdf71 100644 --- a/spring-cloud-netflix-core/src/test/java/org/springframework/cloud/netflix/zuul/filters/route/support/RibbonCommandCauseFallbackPropagationTest.java +++ b/spring-cloud-netflix-core/src/test/java/org/springframework/cloud/netflix/zuul/filters/route/support/RibbonCommandCauseFallbackPropagationTest.java @@ -27,6 +27,7 @@ import com.netflix.client.ClientException; import com.netflix.client.ClientRequest; import com.netflix.client.IResponse; import com.netflix.client.RequestSpecificRetryHandler; +import com.netflix.client.config.DefaultClientConfigImpl; import com.netflix.client.config.IClientConfig; import com.netflix.client.http.HttpResponse; import com.netflix.hystrix.HystrixCommandProperties; @@ -147,7 +148,7 @@ public class RibbonCommandCauseFallbackPropagationTest { AbstractLoadBalancerAwareClient client, ZuulFallbackProvider fallbackProvider, int timeout) { // different name is used because of properties caching - super(getSetter("testCommand" + UUID.randomUUID(), new ZuulProperties()) + super(getSetter("testCommand" + UUID.randomUUID(), new ZuulProperties(), new DefaultClientConfigImpl()) .andCommandPropertiesDefaults(defauts(timeout)), client, null, fallbackProvider, null); }