From 798a4bb2b91b54bbcea21e602bb95080b0e58877 Mon Sep 17 00:00:00 2001 From: JagmohanSharma Date: Tue, 24 Oct 2017 22:48:34 +0530 Subject: [PATCH 1/3] https://github.com/spring-cloud/spring-cloud-netflix/issues/2387 (#2388) In order to fix issue with TPS in hystrix-dashboard threadpool section, removing average calculation for propertyValue_metricsRollingStatisticalWindowInMilliseconds. This was done for hystrixCommand.js previously but not for hystrixThreadpool.js. --- .../hystrix/components/hystrixThreadPool/hystrixThreadPool.js | 3 --- 1 file changed, 3 deletions(-) diff --git a/spring-cloud-netflix-hystrix-dashboard/src/main/resources/static/hystrix/components/hystrixThreadPool/hystrixThreadPool.js b/spring-cloud-netflix-hystrix-dashboard/src/main/resources/static/hystrix/components/hystrixThreadPool/hystrixThreadPool.js index 9f6a3326..851562bf 100644 --- a/spring-cloud-netflix-hystrix-dashboard/src/main/resources/static/hystrix/components/hystrixThreadPool/hystrixThreadPool.js +++ b/spring-cloud-netflix-hystrix-dashboard/src/main/resources/static/hystrix/components/hystrixThreadPool/hystrixThreadPool.js @@ -91,9 +91,6 @@ function converAllAvg(data) { convertAvg(data, "propertyValue_queueSizeRejectionThreshold", false); - - // the following will break when it becomes a compound string if the property is dynamically changed - convertAvg(data, "propertyValue_metricsRollingStatisticalWindowInMilliseconds", false); } function convertAvg(data, key, decimal) { From 271bfcfb31e83e77500d158dff50038d4f5afce8 Mon Sep 17 00:00:00 2001 From: Spencer Gibb Date: Fri, 27 Oct 2017 18:43:24 -0400 Subject: [PATCH 2/3] Fix tests --- ...bbonAutoConfigurationIntegrationTests.java | 2 -- .../ribbon/SpringClientFactoryTests.java | 1 + .../cloud/netflix/test/TestUtils.java | 33 ------------------- .../netflix/zuul/FiltersEndpointTests.java | 2 ++ 4 files changed, 3 insertions(+), 35 deletions(-) delete mode 100644 spring-cloud-netflix-core/src/test/java/org/springframework/cloud/netflix/test/TestUtils.java diff --git a/spring-cloud-netflix-core/src/test/java/org/springframework/cloud/netflix/ribbon/RibbonAutoConfigurationIntegrationTests.java b/spring-cloud-netflix-core/src/test/java/org/springframework/cloud/netflix/ribbon/RibbonAutoConfigurationIntegrationTests.java index 23e2b09f..8c62c872 100644 --- a/spring-cloud-netflix-core/src/test/java/org/springframework/cloud/netflix/ribbon/RibbonAutoConfigurationIntegrationTests.java +++ b/spring-cloud-netflix-core/src/test/java/org/springframework/cloud/netflix/ribbon/RibbonAutoConfigurationIntegrationTests.java @@ -23,7 +23,6 @@ import org.springframework.boot.autoconfigure.context.PropertyPlaceholderAutoCon import org.springframework.boot.test.context.SpringBootTest; import org.springframework.cloud.netflix.archaius.ArchaiusAutoConfiguration; import org.springframework.cloud.netflix.ribbon.RibbonAutoConfigurationIntegrationTests.TestConfiguration; -import org.springframework.cloud.netflix.test.TestUtils; import org.springframework.context.annotation.Configuration; import org.springframework.context.annotation.Import; import org.springframework.test.annotation.DirtiesContext; @@ -47,7 +46,6 @@ public class RibbonAutoConfigurationIntegrationTests { @Test public void serverListIsConfigured() throws Exception { - TestUtils.assumeTestIgnored(RibbonAutoConfigurationIntegrationTests.class); IClientConfig config = this.factory.getClientConfig("client"); assertEquals(25000, config.getPropertyAsInteger(CommonClientConfigKey.ConnectTimeout, 3000)); diff --git a/spring-cloud-netflix-core/src/test/java/org/springframework/cloud/netflix/ribbon/SpringClientFactoryTests.java b/spring-cloud-netflix-core/src/test/java/org/springframework/cloud/netflix/ribbon/SpringClientFactoryTests.java index b927b5f8..045ffa02 100644 --- a/spring-cloud-netflix-core/src/test/java/org/springframework/cloud/netflix/ribbon/SpringClientFactoryTests.java +++ b/spring-cloud-netflix-core/src/test/java/org/springframework/cloud/netflix/ribbon/SpringClientFactoryTests.java @@ -93,6 +93,7 @@ public class SpringClientFactoryTests { ApacheHttpClient4 jerseyClient = (ApacheHttpClient4) client.getJerseyClient(); assertEquals(CookiePolicy.IGNORE_COOKIES, jerseyClient.getClientHandler() .getHttpClient().getParams().getParameter(ClientPNames.COOKIE_POLICY)); + parent.close(); factory.destroy(); } diff --git a/spring-cloud-netflix-core/src/test/java/org/springframework/cloud/netflix/test/TestUtils.java b/spring-cloud-netflix-core/src/test/java/org/springframework/cloud/netflix/test/TestUtils.java deleted file mode 100644 index 60f6af54..00000000 --- a/spring-cloud-netflix-core/src/test/java/org/springframework/cloud/netflix/test/TestUtils.java +++ /dev/null @@ -1,33 +0,0 @@ -/* - * Copyright 2013-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.test; - -import static org.hamcrest.Matchers.containsString; -import static org.hamcrest.Matchers.not; -import static org.junit.Assume.assumeThat; - -public class TestUtils { - public static void assumeTestIgnored(Class clazz) { - assumeTestIgnored(clazz.getSimpleName()); - } - - public static void assumeTestIgnored(String name) { - assumeThat("Test ignored", - System.getenv("SPRING_CLOUD_NETFLIX_IGNORE_TESTS"), - not(containsString(name))); - } -} diff --git a/spring-cloud-netflix-core/src/test/java/org/springframework/cloud/netflix/zuul/FiltersEndpointTests.java b/spring-cloud-netflix-core/src/test/java/org/springframework/cloud/netflix/zuul/FiltersEndpointTests.java index 6fc98430..5d44590e 100644 --- a/spring-cloud-netflix-core/src/test/java/org/springframework/cloud/netflix/zuul/FiltersEndpointTests.java +++ b/spring-cloud-netflix-core/src/test/java/org/springframework/cloud/netflix/zuul/FiltersEndpointTests.java @@ -8,6 +8,7 @@ import org.springframework.boot.autoconfigure.EnableAutoConfiguration; import org.springframework.boot.test.context.SpringBootTest; import org.springframework.context.annotation.Bean; import org.springframework.context.annotation.Configuration; +import org.springframework.test.annotation.DirtiesContext; import org.springframework.test.context.junit4.SpringJUnit4ClassRunner; import org.springframework.web.bind.annotation.RestController; @@ -24,6 +25,7 @@ import static org.junit.Assert.assertEquals; @SpringBootTest(classes = FiltersEndpointApplication.class, webEnvironment = SpringBootTest.WebEnvironment.RANDOM_PORT, value = { "server.contextPath: /app" }) +@DirtiesContext public class FiltersEndpointTests { @Autowired From c0c59320a59c4a03b5b696917a9df2fb7fed9457 Mon Sep 17 00:00:00 2001 From: Daniel Lavoie Date: Mon, 30 Oct 2017 17:40:08 -0400 Subject: [PATCH 3/3] Regression fix for hystrix stream without eureka registration --- .../HystrixStreamAutoConfiguration.java | 11 +++- .../hystrix/stream/HystrixStreamTask.java | 36 ++++++----- ...mAutoConfigurationNoRegistrationTests.java | 61 +++++++++++++++++++ .../HystrixStreamAutoConfigurationTests.java | 53 ++++++++++++++++ 4 files changed, 144 insertions(+), 17 deletions(-) create mode 100644 spring-cloud-netflix-hystrix-stream/src/test/java/org/springframework/cloud/netflix/hystrix/stream/HystrixStreamAutoConfigurationNoRegistrationTests.java create mode 100644 spring-cloud-netflix-hystrix-stream/src/test/java/org/springframework/cloud/netflix/hystrix/stream/HystrixStreamAutoConfigurationTests.java diff --git a/spring-cloud-netflix-hystrix-stream/src/main/java/org/springframework/cloud/netflix/hystrix/stream/HystrixStreamAutoConfiguration.java b/spring-cloud-netflix-hystrix-stream/src/main/java/org/springframework/cloud/netflix/hystrix/stream/HystrixStreamAutoConfiguration.java index 98b17113..f1eaf921 100644 --- a/spring-cloud-netflix-hystrix-stream/src/main/java/org/springframework/cloud/netflix/hystrix/stream/HystrixStreamAutoConfiguration.java +++ b/spring-cloud-netflix-hystrix-stream/src/main/java/org/springframework/cloud/netflix/hystrix/stream/HystrixStreamAutoConfiguration.java @@ -22,7 +22,10 @@ import org.springframework.beans.factory.annotation.Autowired; import org.springframework.boot.autoconfigure.condition.ConditionalOnClass; import org.springframework.boot.autoconfigure.condition.ConditionalOnProperty; import org.springframework.boot.context.properties.EnableConfigurationProperties; +import org.springframework.cloud.client.ServiceInstance; import org.springframework.cloud.client.actuator.HasFeatures; +import org.springframework.cloud.client.discovery.DiscoveryClient; +import org.springframework.cloud.client.discovery.simple.SimpleDiscoveryClient; import org.springframework.cloud.client.serviceregistry.Registration; import org.springframework.cloud.stream.annotation.EnableBinding; import org.springframework.cloud.stream.annotation.Output; @@ -97,8 +100,12 @@ public class HystrixStreamAutoConfiguration { } @Bean - public HystrixStreamTask hystrixStreamTask() { - return new HystrixStreamTask(this.outboundChannel, this.registration, + public HystrixStreamTask hystrixStreamTask(SimpleDiscoveryClient simpleDiscoveryClient) { + ServiceInstance serviceInstance = this.registration; + if (serviceInstance == null) { + serviceInstance = simpleDiscoveryClient.getLocalServiceInstance(); + } + return new HystrixStreamTask(this.outboundChannel, serviceInstance, this.properties); } diff --git a/spring-cloud-netflix-hystrix-stream/src/main/java/org/springframework/cloud/netflix/hystrix/stream/HystrixStreamTask.java b/spring-cloud-netflix-hystrix-stream/src/main/java/org/springframework/cloud/netflix/hystrix/stream/HystrixStreamTask.java index bd2e23fa..7a3a9252 100644 --- a/spring-cloud-netflix-hystrix-stream/src/main/java/org/springframework/cloud/netflix/hystrix/stream/HystrixStreamTask.java +++ b/spring-cloud-netflix-hystrix-stream/src/main/java/org/springframework/cloud/netflix/hystrix/stream/HystrixStreamTask.java @@ -22,6 +22,18 @@ import java.util.ArrayList; import java.util.Collection; import java.util.concurrent.LinkedBlockingQueue; +import org.apache.commons.logging.Log; +import org.apache.commons.logging.LogFactory; +import org.springframework.beans.BeansException; +import org.springframework.cloud.client.ServiceInstance; +import org.springframework.context.ApplicationContext; +import org.springframework.context.ApplicationContextAware; +import org.springframework.messaging.MessageChannel; +import org.springframework.messaging.MessageHeaders; +import org.springframework.messaging.support.MessageBuilder; +import org.springframework.scheduling.annotation.Scheduled; +import org.springframework.util.Assert; + import com.fasterxml.jackson.core.JsonFactory; import com.fasterxml.jackson.core.JsonGenerator; import com.netflix.hystrix.HystrixCircuitBreaker; @@ -32,19 +44,6 @@ import com.netflix.hystrix.HystrixThreadPoolKey; import com.netflix.hystrix.HystrixThreadPoolMetrics; import com.netflix.hystrix.util.HystrixRollingNumberEvent; -import org.apache.commons.logging.Log; -import org.apache.commons.logging.LogFactory; - -import org.springframework.beans.BeansException; -import org.springframework.cloud.client.ServiceInstance; -import org.springframework.cloud.client.serviceregistry.Registration; -import org.springframework.context.ApplicationContext; -import org.springframework.context.ApplicationContextAware; -import org.springframework.messaging.MessageChannel; -import org.springframework.messaging.MessageHeaders; -import org.springframework.messaging.support.MessageBuilder; -import org.springframework.scheduling.annotation.Scheduled; - /** * @author Spencer Gibb * @@ -57,7 +56,7 @@ public class HystrixStreamTask implements ApplicationContextAware { private MessageChannel outboundChannel; - private Registration registration; + private ServiceInstance registration; private HystrixStreamProperties properties; @@ -69,13 +68,20 @@ public class HystrixStreamTask implements ApplicationContextAware { private final JsonFactory jsonFactory = new JsonFactory(); public HystrixStreamTask(MessageChannel outboundChannel, - Registration registration, HystrixStreamProperties properties) { + ServiceInstance registration, HystrixStreamProperties properties) { + Assert.notNull(outboundChannel, "outboundChannel may not be null"); + Assert.notNull(registration, "registration may not be null"); + Assert.notNull(properties, "properties may not be null"); this.outboundChannel = outboundChannel; this.registration = registration; this.properties = properties; this.jsonMetrics = new LinkedBlockingQueue<>(properties.getSize()); } + /* for testing */ ServiceInstance getRegistration() { + return registration; + } + @Override public void setApplicationContext(ApplicationContext applicationContext) throws BeansException { diff --git a/spring-cloud-netflix-hystrix-stream/src/test/java/org/springframework/cloud/netflix/hystrix/stream/HystrixStreamAutoConfigurationNoRegistrationTests.java b/spring-cloud-netflix-hystrix-stream/src/test/java/org/springframework/cloud/netflix/hystrix/stream/HystrixStreamAutoConfigurationNoRegistrationTests.java new file mode 100644 index 00000000..dc18ccb8 --- /dev/null +++ b/spring-cloud-netflix-hystrix-stream/src/test/java/org/springframework/cloud/netflix/hystrix/stream/HystrixStreamAutoConfigurationNoRegistrationTests.java @@ -0,0 +1,61 @@ +/* + * Copyright 2013-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.hystrix.stream; + +import org.junit.Test; +import org.junit.runner.RunWith; +import org.springframework.beans.factory.annotation.Autowired; +import org.springframework.boot.SpringBootConfiguration; +import org.springframework.boot.autoconfigure.EnableAutoConfiguration; +import org.springframework.boot.test.context.SpringBootTest; +import org.springframework.cloud.client.discovery.simple.SimpleDiscoveryClient; +import org.springframework.cloud.client.serviceregistry.Registration; +import org.springframework.test.annotation.DirtiesContext; +import org.springframework.test.context.junit4.SpringRunner; + +import static org.assertj.core.api.Assertions.assertThat; + +/** + * @author Spencer Gibb + */ +@RunWith(SpringRunner.class) +@SpringBootTest("eureka.client.enabled=false") +@DirtiesContext +public class HystrixStreamAutoConfigurationNoRegistrationTests { + + @Autowired + HystrixStreamTask task; + + @Autowired(required = false) + Registration registration; + + @Autowired + SimpleDiscoveryClient simpleDiscoveryClient; + + @Test + public void withoutRegistrationWorks() throws Exception { + assertThat(this.registration).isNull(); + assertThat(this.simpleDiscoveryClient).isNotNull(); + assertThat(task.getRegistration()).isEqualTo(this.simpleDiscoveryClient.getLocalServiceInstance()); + } + + @EnableAutoConfiguration + @SpringBootConfiguration + protected static class Config { + } + +} diff --git a/spring-cloud-netflix-hystrix-stream/src/test/java/org/springframework/cloud/netflix/hystrix/stream/HystrixStreamAutoConfigurationTests.java b/spring-cloud-netflix-hystrix-stream/src/test/java/org/springframework/cloud/netflix/hystrix/stream/HystrixStreamAutoConfigurationTests.java new file mode 100644 index 00000000..8473aae0 --- /dev/null +++ b/spring-cloud-netflix-hystrix-stream/src/test/java/org/springframework/cloud/netflix/hystrix/stream/HystrixStreamAutoConfigurationTests.java @@ -0,0 +1,53 @@ +/* + * Copyright 2013-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.hystrix.stream; + +import org.junit.Test; +import org.junit.runner.RunWith; +import org.springframework.beans.factory.annotation.Autowired; +import org.springframework.boot.SpringBootConfiguration; +import org.springframework.boot.autoconfigure.EnableAutoConfiguration; +import org.springframework.cloud.client.serviceregistry.Registration; +import org.springframework.test.annotation.DirtiesContext; +import org.springframework.test.context.junit4.SpringRunner; + +import static org.assertj.core.api.Assertions.assertThat; + +/** + * @author Spencer Gibb + */ +@RunWith(SpringRunner.class) +@DirtiesContext +public class HystrixStreamAutoConfigurationTests { + + @Autowired + HystrixStreamTask task; + + @Autowired + Registration registration; + + @Test + public void withRegistrationWorks() throws Exception { + assertThat(task.getRegistration()).isEqualTo(this.registration); + } + + @EnableAutoConfiguration + @SpringBootConfiguration + protected static class Config { + } + +}