From 8e11514e7c4063b9432c7625a98b2e2e76098461 Mon Sep 17 00:00:00 2001
From: Yongsung Yoon <yongsung.yoon@gmail.com>
Date: Sat, 1 Jul 2017 09:03:52 +0900
Subject: [PATCH] Add options for RibbonCommand to use separate thread pools
 for hystrix (#2074)

* Add options for RibbonCommand to use separate thread pools for hystrix

* Remove lombok annotation and Add getter/setter for ZuulProperties$HystrixThreadPool

* Add doc about how to configure hystrix thread pools in Zuul Developer Guide

* Add testcases for HystrixThreadPoolKey of RibbonCommand

* Tiny change about if block in AbstractRibbonCommand
---
 .../main/asciidoc/spring-cloud-netflix.adoc   |  22 ++++
 .../netflix/zuul/filters/ZuulProperties.java  |  35 ++++++
 .../route/support/AbstractRibbonCommand.java  |  13 ++-
 .../zuul/filters/ZuulPropertiesTests.java     |   6 +
 ...ibbonCommandHystrixThreadPoolKeyTests.java | 104 ++++++++++++++++++
 5 files changed, 175 insertions(+), 5 deletions(-)
 create mode 100644 spring-cloud-netflix-core/src/test/java/org/springframework/cloud/netflix/zuul/filters/route/support/RibbonCommandHystrixThreadPoolKeyTests.java

diff --git a/docs/src/main/asciidoc/spring-cloud-netflix.adoc b/docs/src/main/asciidoc/spring-cloud-netflix.adoc
index 3b0dd846..99a1bc65 100644
--- a/docs/src/main/asciidoc/spring-cloud-netflix.adoc
+++ b/docs/src/main/asciidoc/spring-cloud-netflix.adoc
@@ -886,6 +886,28 @@ ribbon:
     clients: client1, client2, client3
 ----
 
+[[how-to-configure-hystrix-thread-pools]]
+=== How to Configure Hystrix thread pools
+If you change `zuul.ribbonIsolationStrategy` to THREAD, the thread isolation strategy for Hystrix will be used for all routes. In this case, the HystrixThreadPoolKey is set to "RibbonCommand" as default. It means that HystrixCommands for all routes will be executed in the same Hystrix thread pool. This behavior can be changed using the following configuration and it will result in HystrixCommands being executed in the Hystrix thread pool for each route.
+
+.application.yml
+----
+zuul:
+  threadPool:
+    useSeparateThreadPools: true
+----
+
+The default HystrixThreadPoolKey in this case is same with service ID for each route. To add a prefix to HystrixThreadPoolKey, set `zuul.threadPool.threadPoolKeyPrefix` to a value that you want to add. For example:
+
+.application.yml
+----
+zuul:
+  threadPool:
+    useSeparateThreadPools: true
+    threadPoolKeyPrefix: zuulgw
+----
+
+
 
 [[spring-cloud-feign]]
 == Declarative REST Client: Feign
diff --git a/spring-cloud-netflix-core/src/main/java/org/springframework/cloud/netflix/zuul/filters/ZuulProperties.java b/spring-cloud-netflix-core/src/main/java/org/springframework/cloud/netflix/zuul/filters/ZuulProperties.java
index 8c2d40ad..0f1d3f34 100644
--- a/spring-cloud-netflix-core/src/main/java/org/springframework/cloud/netflix/zuul/filters/ZuulProperties.java
+++ b/spring-cloud-netflix-core/src/main/java/org/springframework/cloud/netflix/zuul/filters/ZuulProperties.java
@@ -160,6 +160,8 @@ public class ZuulProperties {
 	private ExecutionIsolationStrategy ribbonIsolationStrategy = SEMAPHORE;
 	
 	private HystrixSemaphore semaphore = new HystrixSemaphore();
+
+	private HystrixThreadPool threadPool = new HystrixThreadPool();
 	
 	public Set<String> getIgnoredHeaders() {
 		Set<String> ignoredHeaders = new LinkedHashSet<>(this.ignoredHeaders);
@@ -357,6 +359,39 @@ public class ZuulProperties {
 		
 	}
 
+	public static class HystrixThreadPool {
+		/**
+		 * Flag to determine whether RibbonCommands should use separate thread pools for hystrix.
+		 * By setting to true, RibbonCommands will be executed in a hystrix's thread pool that it is associated with.
+		 * Each RibbonCommand will be associated with a thread pool according to its commandKey (serviceId).
+		 * As default, all commands will be executed in a single thread pool whose threadPoolKey is "RibbonCommand".
+		 * This property is only applicable when using THREAD as ribbonIsolationStrategy
+		 */
+		private boolean useSeparateThreadPools = false;
+
+		/**
+		 * A prefix for HystrixThreadPoolKey of hystrix's thread pool that is allocated to each service Id.
+		 * This property is only applicable when using THREAD as ribbonIsolationStrategy and useSeparateThreadPools = true
+		 */
+		private String threadPoolKeyPrefix = "";
+
+		public boolean isUseSeparateThreadPools() {
+			return useSeparateThreadPools;
+		}
+
+		public void setUseSeparateThreadPools(boolean useSeparateThreadPools) {
+			this.useSeparateThreadPools = useSeparateThreadPools;
+		}
+
+		public String getThreadPoolKeyPrefix() {
+			return threadPoolKeyPrefix;
+		}
+
+		public void setThreadPoolKeyPrefix(String threadPoolKeyPrefix) {
+			this.threadPoolKeyPrefix = threadPoolKeyPrefix;
+		}
+	}
+
 	public String getServletPattern() {
 		String path = this.servletPath;
 		if (!path.startsWith("/")) {
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 3c157929..5431405e 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
@@ -34,6 +34,7 @@ import com.netflix.hystrix.HystrixCommandGroupKey;
 import com.netflix.hystrix.HystrixCommandKey;
 import com.netflix.hystrix.HystrixCommandProperties;
 import com.netflix.hystrix.HystrixCommandProperties.ExecutionIsolationStrategy;
+import com.netflix.hystrix.HystrixThreadPoolKey;
 import com.netflix.zuul.constants.ZuulConstants;
 import com.netflix.zuul.context.RequestContext;
 
@@ -78,6 +79,9 @@ public abstract class AbstractRibbonCommand<LBC extends AbstractLoadBalancerAwar
 			ZuulProperties zuulProperties) {
 
 		// @formatter:off
+		Setter commandSetter = Setter.withGroupKey(HystrixCommandGroupKey.Factory.asKey("RibbonCommand"))
+								.andCommandKey(HystrixCommandKey.Factory.asKey(commandKey));
+
 		final HystrixCommandProperties.Setter setter = HystrixCommandProperties.Setter()
 				.withExecutionIsolationStrategy(zuulProperties.getRibbonIsolationStrategy());
 		if (zuulProperties.getRibbonIsolationStrategy() == ExecutionIsolationStrategy.SEMAPHORE){
@@ -87,13 +91,12 @@ public abstract class AbstractRibbonCommand<LBC extends AbstractLoadBalancerAwar
 			final DynamicIntProperty value = DynamicPropertyFactory.getInstance()
 					.getIntProperty(name, zuulProperties.getSemaphore().getMaxSemaphores());
 			setter.withExecutionIsolationSemaphoreMaxConcurrentRequests(value.get());
-		} else	{
-			// TODO Find out is some parameters can be set here
+		} else if (zuulProperties.getThreadPool().isUseSeparateThreadPools()) {
+			final String threadPoolKey = zuulProperties.getThreadPool().getThreadPoolKeyPrefix() + commandKey;
+			commandSetter.andThreadPoolKey(HystrixThreadPoolKey.Factory.asKey(threadPoolKey));
 		}
 		
-		return Setter.withGroupKey(HystrixCommandGroupKey.Factory.asKey("RibbonCommand"))
-				.andCommandKey(HystrixCommandKey.Factory.asKey(commandKey))
-				.andCommandPropertiesDefaults(setter);
+		return commandSetter.andCommandPropertiesDefaults(setter);
 		// @formatter:on
 	}
 
diff --git a/spring-cloud-netflix-core/src/test/java/org/springframework/cloud/netflix/zuul/filters/ZuulPropertiesTests.java b/spring-cloud-netflix-core/src/test/java/org/springframework/cloud/netflix/zuul/filters/ZuulPropertiesTests.java
index e0b737ee..4678fa3a 100644
--- a/spring-cloud-netflix-core/src/test/java/org/springframework/cloud/netflix/zuul/filters/ZuulPropertiesTests.java
+++ b/spring-cloud-netflix-core/src/test/java/org/springframework/cloud/netflix/zuul/filters/ZuulPropertiesTests.java
@@ -24,6 +24,7 @@ import org.junit.Before;
 import org.junit.Test;
 import org.springframework.cloud.netflix.zuul.filters.ZuulProperties.ZuulRoute;
 
+import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.assertTrue;
 
@@ -103,4 +104,9 @@ public class ZuulPropertiesTests {
 		assertFalse(this.zuul.getSensitiveHeaders().contains("Cookie"));
 	}
 
+	@Test
+	public void defaultHystrixThreadPool() {
+		assertFalse(this.zuul.getThreadPool().isUseSeparateThreadPools());
+		assertEquals("", this.zuul.getThreadPool().getThreadPoolKeyPrefix());
+	}
 }
diff --git a/spring-cloud-netflix-core/src/test/java/org/springframework/cloud/netflix/zuul/filters/route/support/RibbonCommandHystrixThreadPoolKeyTests.java b/spring-cloud-netflix-core/src/test/java/org/springframework/cloud/netflix/zuul/filters/route/support/RibbonCommandHystrixThreadPoolKeyTests.java
new file mode 100644
index 00000000..bc095a07
--- /dev/null
+++ b/spring-cloud-netflix-core/src/test/java/org/springframework/cloud/netflix/zuul/filters/route/support/RibbonCommandHystrixThreadPoolKeyTests.java
@@ -0,0 +1,104 @@
+/*
+ * Copyright 2017 the original author or authors.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.springframework.cloud.netflix.zuul.filters.route.support;
+
+import com.netflix.client.ClientRequest;
+import com.netflix.hystrix.HystrixCommandProperties;
+import org.junit.Before;
+import org.junit.Test;
+import org.springframework.cloud.netflix.zuul.filters.ZuulProperties;
+
+import static org.assertj.core.api.Assertions.assertThat;
+
+/**
+ * @author Yongsung Yoon
+ */
+public class RibbonCommandHystrixThreadPoolKeyTests {
+
+	private ZuulProperties zuulProperties;
+
+	@Before
+	public void setUp() throws Exception {
+		zuulProperties = new ZuulProperties();
+	}
+
+	@Test
+	public void testDefaultHystrixThreadPoolKey() throws Exception {
+		zuulProperties.setRibbonIsolationStrategy(HystrixCommandProperties.ExecutionIsolationStrategy.THREAD);
+
+		TestRibbonCommand ribbonCommand1 = new TestRibbonCommand("testCommand1", zuulProperties);
+		TestRibbonCommand ribbonCommand2 = new TestRibbonCommand("testCommand2", zuulProperties);
+
+		// CommandGroupKey should be used as ThreadPoolKey as default.
+		assertThat(ribbonCommand1.getThreadPoolKey().name()).isEqualTo(ribbonCommand1.getCommandGroup().name());
+		assertThat(ribbonCommand2.getThreadPoolKey().name()).isEqualTo(ribbonCommand2.getCommandGroup().name());
+	}
+
+	@Test
+	public void testUseSeparateThreadPools() throws Exception {
+		zuulProperties.setRibbonIsolationStrategy(HystrixCommandProperties.ExecutionIsolationStrategy.THREAD);
+		zuulProperties.getThreadPool().setUseSeparateThreadPools(true);
+
+		TestRibbonCommand ribbonCommand1 = new TestRibbonCommand("testCommand1", zuulProperties);
+		TestRibbonCommand ribbonCommand2 = new TestRibbonCommand("testCommand2", zuulProperties);
+
+		assertThat(ribbonCommand1.getThreadPoolKey().name()).isEqualTo("testCommand1");
+		assertThat(ribbonCommand2.getThreadPoolKey().name()).isEqualTo("testCommand2");
+	}
+
+	@Test
+	public void testThreadPoolKeyPrefix() throws Exception {
+		final String prefix = "zuulgw-";
+
+		zuulProperties.setRibbonIsolationStrategy(HystrixCommandProperties.ExecutionIsolationStrategy.THREAD);
+		zuulProperties.getThreadPool().setUseSeparateThreadPools(true);
+		zuulProperties.getThreadPool().setThreadPoolKeyPrefix(prefix);
+
+		TestRibbonCommand ribbonCommand1 = new TestRibbonCommand("testCommand1", zuulProperties);
+		TestRibbonCommand ribbonCommand2 = new TestRibbonCommand("testCommand2", zuulProperties);
+
+		assertThat(ribbonCommand1.getThreadPoolKey().name()).isEqualTo(prefix + "testCommand1");
+		assertThat(ribbonCommand2.getThreadPoolKey().name()).isEqualTo(prefix + "testCommand2");
+	}
+
+	@Test
+	public void testNoSideEffectOnSemaphoreIsolation() throws Exception {
+		final String prefix = "zuulgw-";
+
+		zuulProperties.setRibbonIsolationStrategy(HystrixCommandProperties.ExecutionIsolationStrategy.SEMAPHORE);
+		zuulProperties.getThreadPool().setUseSeparateThreadPools(true);
+		zuulProperties.getThreadPool().setThreadPoolKeyPrefix(prefix);
+
+		TestRibbonCommand ribbonCommand1 = new TestRibbonCommand("testCommand1", zuulProperties);
+		TestRibbonCommand ribbonCommand2 = new TestRibbonCommand("testCommand2", zuulProperties);
+
+		// There should be no side effect on semaphore isolation
+		assertThat(ribbonCommand1.getThreadPoolKey().name()).isEqualTo(ribbonCommand1.getCommandGroup().name());
+		assertThat(ribbonCommand2.getThreadPoolKey().name()).isEqualTo(ribbonCommand2.getCommandGroup().name());
+	}
+
+	public static class TestRibbonCommand extends AbstractRibbonCommand {
+		public TestRibbonCommand(String commandKey, ZuulProperties zuulProperties) {
+			super(commandKey, null, null, zuulProperties);
+		}
+
+		@Override
+		protected ClientRequest createRequest() throws Exception {
+			return null;
+		}
+	}
+}
\ No newline at end of file