From 05351b78097be2b2ed215619197dfc80e5fa8aac Mon Sep 17 00:00:00 2001 From: Tyler Van Gorder Date: Fri, 1 Apr 2016 13:19:05 -0700 Subject: [PATCH] Ribbon retry works & fix Content-Length error. This change requires setting `ribbon.http.client.enabled=true`. This change addresses issue #648 Getting Retry Working In RestTemplate. There is also a fix in place that addresses the issue #357 RestTemplate Error. fixes gh-648 --- .../ribbon/RibbonAutoConfiguration.java | 6 +- .../ribbon/RibbonClientConfiguration.java | 10 + .../RibbonClientHttpRequestFactory.java | 33 +- .../netflix/ribbon/RibbonHttpRequest.java | 46 +-- .../netflix/ribbon/RibbonProperyUtils.java | 33 ++ .../resttemplate/RestTemplateRetryTest.java | 296 ++++++++++++++++++ .../EurekaRibbonClientConfiguration.java | 38 +-- .../EurekaRibbonClientConfigurationTests.java | 9 +- 8 files changed, 381 insertions(+), 90 deletions(-) create mode 100644 spring-cloud-netflix-core/src/main/java/org/springframework/cloud/netflix/ribbon/RibbonProperyUtils.java create mode 100644 spring-cloud-netflix-core/src/test/java/org/springframework/cloud/netflix/resttemplate/RestTemplateRetryTest.java diff --git a/spring-cloud-netflix-core/src/main/java/org/springframework/cloud/netflix/ribbon/RibbonAutoConfiguration.java b/spring-cloud-netflix-core/src/main/java/org/springframework/cloud/netflix/ribbon/RibbonAutoConfiguration.java index 42486ccb..61500e9e 100644 --- a/spring-cloud-netflix-core/src/main/java/org/springframework/cloud/netflix/ribbon/RibbonAutoConfiguration.java +++ b/spring-cloud-netflix-core/src/main/java/org/springframework/cloud/netflix/ribbon/RibbonAutoConfiguration.java @@ -79,9 +79,6 @@ public class RibbonAutoConfiguration { @Autowired private SpringClientFactory springClientFactory; - @Autowired - private LoadBalancerClient loadBalancerClient; - @Bean public RestTemplateCustomizer restTemplateCustomizer( final RibbonClientHttpRequestFactory ribbonClientHttpRequestFactory) { @@ -95,8 +92,7 @@ public class RibbonAutoConfiguration { @Bean public RibbonClientHttpRequestFactory ribbonClientHttpRequestFactory() { - return new RibbonClientHttpRequestFactory(this.springClientFactory, - this.loadBalancerClient); + return new RibbonClientHttpRequestFactory(this.springClientFactory); } } diff --git a/spring-cloud-netflix-core/src/main/java/org/springframework/cloud/netflix/ribbon/RibbonClientConfiguration.java b/spring-cloud-netflix-core/src/main/java/org/springframework/cloud/netflix/ribbon/RibbonClientConfiguration.java index 0f955844..f3dc5671 100644 --- a/spring-cloud-netflix-core/src/main/java/org/springframework/cloud/netflix/ribbon/RibbonClientConfiguration.java +++ b/spring-cloud-netflix-core/src/main/java/org/springframework/cloud/netflix/ribbon/RibbonClientConfiguration.java @@ -18,6 +18,8 @@ package org.springframework.cloud.netflix.ribbon; import java.net.URI; +import javax.annotation.PostConstruct; + import org.apache.http.client.params.ClientPNames; import org.apache.http.client.params.CookiePolicy; import org.springframework.beans.factory.annotation.Value; @@ -48,6 +50,9 @@ import com.netflix.servo.monitor.Monitors; import com.sun.jersey.api.client.Client; import com.sun.jersey.client.apache4.ApacheHttpClient4; +import static com.netflix.client.config.CommonClientConfigKey.DeploymentContextBasedVipAddresses; +import static org.springframework.cloud.netflix.ribbon.RibbonProperyUtils.setRibbonProperty; + /** * @author Dave Syer */ @@ -155,6 +160,11 @@ public class RibbonClientConfiguration { return new DefaultServerIntrospector(); } + @PostConstruct + public void preprocess() { + setRibbonProperty(name, DeploymentContextBasedVipAddresses.key(), name); + } + static class OverrideRestClient extends RestClient { private ServerIntrospector serverIntrospector; diff --git a/spring-cloud-netflix-core/src/main/java/org/springframework/cloud/netflix/ribbon/RibbonClientHttpRequestFactory.java b/spring-cloud-netflix-core/src/main/java/org/springframework/cloud/netflix/ribbon/RibbonClientHttpRequestFactory.java index ca0c2fe1..8fde9b57 100644 --- a/spring-cloud-netflix-core/src/main/java/org/springframework/cloud/netflix/ribbon/RibbonClientHttpRequestFactory.java +++ b/spring-cloud-netflix-core/src/main/java/org/springframework/cloud/netflix/ribbon/RibbonClientHttpRequestFactory.java @@ -19,16 +19,12 @@ package org.springframework.cloud.netflix.ribbon; import java.io.IOException; import java.net.URI; -import org.springframework.cloud.client.ServiceInstance; -import org.springframework.cloud.client.loadbalancer.LoadBalancerClient; -import org.springframework.cloud.netflix.ribbon.RibbonLoadBalancerClient.RibbonServer; import org.springframework.http.HttpMethod; import org.springframework.http.client.ClientHttpRequest; import org.springframework.http.client.ClientHttpRequestFactory; import com.netflix.client.config.IClientConfig; import com.netflix.client.http.HttpRequest; -import com.netflix.loadbalancer.Server; import com.netflix.niws.client.http.RestClient; /** @@ -38,12 +34,8 @@ public class RibbonClientHttpRequestFactory implements ClientHttpRequestFactory private final SpringClientFactory clientFactory; - private LoadBalancerClient loadBalancer; - - public RibbonClientHttpRequestFactory(SpringClientFactory clientFactory, - LoadBalancerClient loadBalancer) { + public RibbonClientHttpRequestFactory(SpringClientFactory clientFactory) { this.clientFactory = clientFactory; - this.loadBalancer = loadBalancer; } @Override @@ -55,28 +47,11 @@ public class RibbonClientHttpRequestFactory implements ClientHttpRequestFactory throw new IOException( "Invalid hostname in the URI [" + originalUri.toASCIIString() + "]"); } - ServiceInstance instance = this.loadBalancer.choose(serviceId); - if (instance == null) { - throw new IllegalStateException("No instances available for " + serviceId); - } - URI uri = this.loadBalancer.reconstructURI(instance, originalUri); - - IClientConfig clientConfig = this.clientFactory - .getClientConfig(instance.getServiceId()); - RestClient client = this.clientFactory.getClient(instance.getServiceId(), - RestClient.class); + IClientConfig clientConfig = this.clientFactory.getClientConfig(serviceId); + RestClient client = this.clientFactory.getClient(serviceId, RestClient.class); HttpRequest.Verb verb = HttpRequest.Verb.valueOf(httpMethod.name()); - RibbonLoadBalancerContext context = this.clientFactory - .getLoadBalancerContext(serviceId); - Server server = null; - if (instance instanceof RibbonServer) { - server = ((RibbonServer) instance).getServer(); - } - - RibbonStatsRecorder statsRecorder = new RibbonStatsRecorder(context, server); - - return new RibbonHttpRequest(uri, verb, client, clientConfig, statsRecorder); + return new RibbonHttpRequest(originalUri, verb, client, clientConfig); } } diff --git a/spring-cloud-netflix-core/src/main/java/org/springframework/cloud/netflix/ribbon/RibbonHttpRequest.java b/spring-cloud-netflix-core/src/main/java/org/springframework/cloud/netflix/ribbon/RibbonHttpRequest.java index 04bbf983..2d68b12d 100644 --- a/spring-cloud-netflix-core/src/main/java/org/springframework/cloud/netflix/ribbon/RibbonHttpRequest.java +++ b/spring-cloud-netflix-core/src/main/java/org/springframework/cloud/netflix/ribbon/RibbonHttpRequest.java @@ -16,21 +16,22 @@ package org.springframework.cloud.netflix.ribbon; -import com.netflix.client.config.IClientConfig; -import com.netflix.client.http.HttpRequest; -import com.netflix.client.http.HttpResponse; -import com.netflix.niws.client.http.RestClient; -import org.springframework.http.HttpHeaders; -import org.springframework.http.HttpMethod; -import org.springframework.http.client.AbstractClientHttpRequest; -import org.springframework.http.client.ClientHttpResponse; - import java.io.ByteArrayOutputStream; import java.io.IOException; import java.io.OutputStream; import java.net.URI; import java.util.List; +import org.springframework.http.HttpHeaders; +import org.springframework.http.HttpMethod; +import org.springframework.http.client.AbstractClientHttpRequest; +import org.springframework.http.client.ClientHttpResponse; + +import com.netflix.client.config.IClientConfig; +import com.netflix.client.http.HttpRequest; +import com.netflix.client.http.HttpResponse; +import com.netflix.niws.client.http.RestClient; + /** * @author Spencer Gibb */ @@ -42,16 +43,14 @@ public class RibbonHttpRequest extends AbstractClientHttpRequest { private HttpRequest.Verb verb; private RestClient client; private IClientConfig config; - private RibbonStatsRecorder statsRecorder; private ByteArrayOutputStream outputStream = null; public RibbonHttpRequest(URI uri, HttpRequest.Verb verb, RestClient client, - IClientConfig config, RibbonStatsRecorder statsRecorder) { + IClientConfig config) { this.uri = uri; this.verb = verb; this.client = client; this.config = config; - this.statsRecorder = statsRecorder; this.builder = HttpRequest.newBuilder().uri(uri).verb(verb); } @@ -83,24 +82,27 @@ public class RibbonHttpRequest extends AbstractClientHttpRequest { builder.entity(outputStream.toByteArray()); } HttpRequest request = builder.build(); - HttpResponse response = client.execute(request, config); - statsRecorder.recordStats(response); + HttpResponse response = client.executeWithLoadBalancer(request, config); return new RibbonHttpResponse(response); } catch (Exception e) { - statsRecorder.recordStats(e); throw new IOException(e); } } private void addHeaders(HttpHeaders headers) { for (String name : headers.keySet()) { - // apache http RequestContent pukes if there is a body and - // the dynamic headers are already present - if (!isDynamic(name)) { - List values = headers.get(name); - for (String value : values) { - builder.header(name, value); - } + // apache http RequestContent pukes if there is a body and + // the dynamic headers are already present + if (isDynamic(name) && outputStream != null) { + continue; + } + //Don't add content-length if the output stream is null. The RibbonClient does this for us. + if (name.equals("Content-Length") && outputStream == null) { + continue; + } + List values = headers.get(name); + for (String value : values) { + builder.header(name, value); } } } diff --git a/spring-cloud-netflix-core/src/main/java/org/springframework/cloud/netflix/ribbon/RibbonProperyUtils.java b/spring-cloud-netflix-core/src/main/java/org/springframework/cloud/netflix/ribbon/RibbonProperyUtils.java new file mode 100644 index 00000000..b4881ede --- /dev/null +++ b/spring-cloud-netflix-core/src/main/java/org/springframework/cloud/netflix/ribbon/RibbonProperyUtils.java @@ -0,0 +1,33 @@ +package org.springframework.cloud.netflix.ribbon; + +import com.netflix.config.ConfigurationManager; +import com.netflix.config.DynamicPropertyFactory; +import com.netflix.config.DynamicStringProperty; + +/** + * @author Spencer Gibb + */ +public class RibbonProperyUtils { + + public static final String VALUE_NOT_SET = "__not__set__"; + + public static final String DEFAULT_NAMESPACE = "ribbon"; + + public static void setRibbonProperty(String serviceId, String suffix, String value) { + // how to set the namespace properly? + String key = getRibbonKey(serviceId, suffix); + DynamicStringProperty property = getProperty(key); + if (property.get().equals(VALUE_NOT_SET)) { + ConfigurationManager.getConfigInstance().setProperty(key, value); + } + } + + public static String getRibbonKey(String serviceId, String suffix) { + return serviceId + "." + DEFAULT_NAMESPACE + "." + suffix; + } + + public static DynamicStringProperty getProperty(String key) { + return DynamicPropertyFactory.getInstance().getStringProperty(key, VALUE_NOT_SET); + } + +} diff --git a/spring-cloud-netflix-core/src/test/java/org/springframework/cloud/netflix/resttemplate/RestTemplateRetryTest.java b/spring-cloud-netflix-core/src/test/java/org/springframework/cloud/netflix/resttemplate/RestTemplateRetryTest.java new file mode 100644 index 00000000..e2abc71e --- /dev/null +++ b/spring-cloud-netflix-core/src/test/java/org/springframework/cloud/netflix/resttemplate/RestTemplateRetryTest.java @@ -0,0 +1,296 @@ +package org.springframework.cloud.netflix.resttemplate; + +import java.net.UnknownHostException; +import java.util.Arrays; +import java.util.concurrent.atomic.AtomicInteger; + +import org.apache.commons.logging.Log; +import org.apache.commons.logging.LogFactory; +import org.junit.Before; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.springframework.beans.factory.annotation.Autowired; +import org.springframework.beans.factory.annotation.Value; +import org.springframework.boot.autoconfigure.EnableAutoConfiguration; +import org.springframework.boot.test.IntegrationTest; +import org.springframework.boot.test.SpringApplicationConfiguration; +import org.springframework.cloud.client.loadbalancer.LoadBalanced; +import org.springframework.cloud.netflix.ribbon.RibbonClient; +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.test.context.web.WebAppConfiguration; +import org.springframework.util.Assert; +import org.springframework.web.bind.annotation.RequestMapping; +import org.springframework.web.bind.annotation.RequestMethod; +import org.springframework.web.bind.annotation.RestController; +import org.springframework.web.client.RestTemplate; + +import com.netflix.client.RetryHandler; +import com.netflix.client.config.IClientConfig; +import com.netflix.loadbalancer.AvailabilityFilteringRule; +import com.netflix.loadbalancer.BaseLoadBalancer; +import com.netflix.loadbalancer.ILoadBalancer; +import com.netflix.loadbalancer.IPing; +import com.netflix.loadbalancer.IRule; +import com.netflix.loadbalancer.LoadBalancerBuilder; +import com.netflix.loadbalancer.LoadBalancerStats; +import com.netflix.loadbalancer.Server; +import com.netflix.loadbalancer.ServerList; +import com.netflix.loadbalancer.ServerStats; +import com.netflix.niws.client.http.HttpClientLoadBalancerErrorHandler; + +@RunWith(SpringJUnit4ClassRunner.class) +@SpringApplicationConfiguration(classes = RestTemplateRetryTest.Application.class) +@WebAppConfiguration +@IntegrationTest({ + "server.port=0", + "spring.application.name=resttemplatetest", + "logging.level.org.springframework.cloud.netflix.resttemplate=DEBUG", + "badClients.ribbon.MaxAutoRetries=0", "badClients.ribbon.ReadTimeout=200", + "badClients.ribbon.MaxAutoRetriesNextServer=10", + "badClients.ribbon.OkToRetryOnAllOperations=true", "ribbon.http.client.enabled" }) +@DirtiesContext +public class RestTemplateRetryTest { + + final private static Log logger = LogFactory.getLog(RestTemplateRetryTest.class); + + @Value("${local.server.port}") + private int port = 0; + + @Autowired + private RestTemplate testClient; + + public RestTemplateRetryTest() { + } + + @Configuration + @EnableAutoConfiguration + @RestController + @RibbonClient(name = "badClients", configuration = LocalBadClientConfiguration.class) + public static class Application { + + private AtomicInteger hits = new AtomicInteger(1); + private AtomicInteger retryHits = new AtomicInteger(1); + + @RequestMapping(method = RequestMethod.GET, value = "/ping") + public int ping() { + return 0; + } + + @RequestMapping(method = RequestMethod.GET, value = "/good") + public int good() { + int lValue = hits.getAndIncrement(); + return lValue; + } + + @RequestMapping(method = RequestMethod.GET, value = "/timeout") + public int timeout() throws Exception { + int lValue = retryHits.getAndIncrement(); + + // Force the good server to have 2 consecutive errors a couple of times. + if (lValue == 2 || lValue == 3 || lValue == 5 || lValue == 6) { + Thread.sleep(500); + } + return lValue; + } + + @RequestMapping(method = RequestMethod.GET, value = "/null") + public int isNull() throws Exception { + throw new NullPointerException("Null"); + } + + @LoadBalanced + @Bean + RestTemplate restTemplate() { + return new RestTemplate(); + } + } + + @Before + public void setup() throws Exception { + // Force Ribbon configuration by making one call. + testClient.getForObject("http://badClients/ping", Integer.class); + } + + @Test + public void testNullPointer() throws Exception { + + LoadBalancerStats stats = LocalBadClientConfiguration.balancer + .getLoadBalancerStats(); + ServerStats badServer1Stats = stats + .getSingleServerStat(LocalBadClientConfiguration.badServer); + ServerStats badServer2Stats = stats + .getSingleServerStat(LocalBadClientConfiguration.badServer2); + ServerStats goodServerStats = stats + .getSingleServerStat(LocalBadClientConfiguration.goodServer); + + badServer1Stats.clearSuccessiveConnectionFailureCount(); + badServer2Stats.clearSuccessiveConnectionFailureCount(); + long targetConnectionCount = goodServerStats.getTotalRequestsCount() + 10; + + // A null pointer should NOT trigger a circuit breaker. + for (int index = 0; index < 10; index++) { + try { + testClient.getForObject("http://badClients/null", Integer.class); + } + catch (Exception exception) { + } + } + logServerStats(LocalBadClientConfiguration.badServer); + logServerStats(LocalBadClientConfiguration.badServer2); + logServerStats(LocalBadClientConfiguration.goodServer); + + Assert.isTrue(badServer1Stats.isCircuitBreakerTripped()); + Assert.isTrue(badServer2Stats.isCircuitBreakerTripped()); + Assert.isTrue(goodServerStats.getTotalRequestsCount() == targetConnectionCount); + + // Wait for any timeout thread to finish. + + } + + private void logServerStats(Server server) { + LoadBalancerStats stats = LocalBadClientConfiguration.balancer + .getLoadBalancerStats(); + ServerStats serverStats = stats.getSingleServerStat(server); + logger.debug("Server : " + server.toString() + " : Total Count == " + + serverStats.getTotalRequestsCount() + ", Failure Count == " + + serverStats.getFailureCount() + ", Successive Connection Failure == " + + serverStats.getSuccessiveConnectionFailureCount() + + ", Circuit Breaker ? == " + serverStats.isCircuitBreakerTripped()); + } + + @Test + public void testRestRetries() { + + LoadBalancerStats stats = LocalBadClientConfiguration.balancer + .getLoadBalancerStats(); + ServerStats badServer1Stats = stats + .getSingleServerStat(LocalBadClientConfiguration.badServer); + ServerStats badServer2Stats = stats + .getSingleServerStat(LocalBadClientConfiguration.badServer2); + ServerStats goodServerStats = stats + .getSingleServerStat(LocalBadClientConfiguration.goodServer); + + badServer1Stats.clearSuccessiveConnectionFailureCount(); + badServer2Stats.clearSuccessiveConnectionFailureCount(); + long targetConnectionCount = goodServerStats.getTotalRequestsCount() + 20; + + int hits = 0; + + for (int index = 0; index < 20; index++) { + hits = testClient.getForObject("http://badClients/good", Integer.class); + } + + logServerStats(LocalBadClientConfiguration.badServer); + logServerStats(LocalBadClientConfiguration.badServer2); + logServerStats(LocalBadClientConfiguration.goodServer); + + Assert.isTrue(badServer1Stats.isCircuitBreakerTripped()); + Assert.isTrue(badServer2Stats.isCircuitBreakerTripped()); + Assert.isTrue(goodServerStats.getTotalRequestsCount() == targetConnectionCount); + Assert.isTrue(hits == 20); + System.out.println("Retry Hits: " + hits); + } + + @Test + public void testRestRetriesWithReadTimeout() throws Exception { + + LoadBalancerStats stats = LocalBadClientConfiguration.balancer + .getLoadBalancerStats(); + ServerStats badServer1Stats = stats + .getSingleServerStat(LocalBadClientConfiguration.badServer); + ServerStats badServer2Stats = stats + .getSingleServerStat(LocalBadClientConfiguration.badServer2); + ServerStats goodServerStats = stats + .getSingleServerStat(LocalBadClientConfiguration.goodServer); + + badServer1Stats.clearSuccessiveConnectionFailureCount(); + badServer2Stats.clearSuccessiveConnectionFailureCount(); + Assert.isTrue(!badServer1Stats.isCircuitBreakerTripped()); + Assert.isTrue(!badServer2Stats.isCircuitBreakerTripped()); + + int hits = 0; + + for (int index = 0; index < 15; index++) { + hits = testClient.getForObject("http://badClients/timeout", Integer.class); + } + logServerStats(LocalBadClientConfiguration.badServer); + logServerStats(LocalBadClientConfiguration.badServer2); + logServerStats(LocalBadClientConfiguration.goodServer); + + Assert.isTrue(badServer1Stats.isCircuitBreakerTripped()); + Assert.isTrue(badServer2Stats.isCircuitBreakerTripped()); + Assert.isTrue(!goodServerStats.isCircuitBreakerTripped()); + + // 15 + 4 timeouts. See the endpoint for timeout conditions. + Assert.isTrue(hits == 19); + + // Wait for any timeout thread to finish. + Thread.sleep(600); + + } + +} + +// Load balancer with fixed server list for "local" pointing to localhost +// and some bogus servers are thrown in to test retry +@Configuration +class LocalBadClientConfiguration { + + static BaseLoadBalancer balancer; + static Server goodServer; + static Server badServer; + static Server badServer2; + + public LocalBadClientConfiguration() { + } + + @Value("${local.server.port}") + private int port = 0; + + @Bean + public IRule loadBalancerRule() { + // This is a good place to try different load balancing rules and how those rules + // behave in failure + // states: BestAvailableRule, WeightedResponseTimeRule, etc + + // This rule just uses a round robin and will skip servers that are in circuit + // breaker state. + return new AvailabilityFilteringRule(); + + } + + @Bean + public ILoadBalancer ribbonLoadBalancer(IClientConfig config, + ServerList serverList, IRule rule, IPing ping) { + + goodServer = new Server("localhost", this.port); + badServer = new Server("mybadhost", 10001); + badServer2 = new Server("localhost", -1); + + balancer = LoadBalancerBuilder + .newBuilder() + .withClientConfig(config) + .withRule(rule) + .withPing(ping) + .buildFixedServerListLoadBalancer( + Arrays.asList(badServer, badServer2, goodServer)); + return balancer; + } + + @Bean + public RetryHandler retryHandler() { + return new OverrideRetryHandler(); + } + + static class OverrideRetryHandler extends HttpClientLoadBalancerErrorHandler { + public OverrideRetryHandler() { + circuitRelated.add(UnknownHostException.class); + retriable.add(UnknownHostException.class); + + } + } + +} diff --git a/spring-cloud-netflix-eureka-client/src/main/java/org/springframework/cloud/netflix/ribbon/eureka/EurekaRibbonClientConfiguration.java b/spring-cloud-netflix-eureka-client/src/main/java/org/springframework/cloud/netflix/ribbon/eureka/EurekaRibbonClientConfiguration.java index 7f44df4b..d551316b 100644 --- a/spring-cloud-netflix-eureka-client/src/main/java/org/springframework/cloud/netflix/ribbon/eureka/EurekaRibbonClientConfiguration.java +++ b/spring-cloud-netflix-eureka-client/src/main/java/org/springframework/cloud/netflix/ribbon/eureka/EurekaRibbonClientConfiguration.java @@ -16,13 +16,8 @@ package org.springframework.cloud.netflix.ribbon.eureka; -import static com.netflix.client.config.CommonClientConfigKey.DeploymentContextBasedVipAddresses; -import static com.netflix.client.config.CommonClientConfigKey.EnableZoneAffinity; - import javax.annotation.PostConstruct; -import lombok.extern.apachecommons.CommonsLog; - import org.springframework.beans.factory.annotation.Autowired; import org.springframework.beans.factory.annotation.Value; import org.springframework.boot.autoconfigure.condition.ConditionalOnMissingBean; @@ -35,14 +30,18 @@ import com.netflix.appinfo.EurekaInstanceConfig; import com.netflix.client.config.IClientConfig; import com.netflix.config.ConfigurationManager; import com.netflix.config.DeploymentContext.ContextKey; -import com.netflix.config.DynamicPropertyFactory; -import com.netflix.config.DynamicStringProperty; import com.netflix.discovery.EurekaClientConfig; import com.netflix.loadbalancer.IPing; import com.netflix.loadbalancer.ServerList; import com.netflix.niws.loadbalancer.DiscoveryEnabledNIWSServerList; import com.netflix.niws.loadbalancer.NIWSDiscoveryPing; +import lombok.extern.apachecommons.CommonsLog; + +import static com.netflix.client.config.CommonClientConfigKey.DeploymentContextBasedVipAddresses; +import static com.netflix.client.config.CommonClientConfigKey.EnableZoneAffinity; +import static org.springframework.cloud.netflix.ribbon.RibbonProperyUtils.setRibbonProperty; + /** * Preprocessor that configures defaults for eureka-discovered ribbon clients. Such as: * @zone, NIWSServerListClassName, DeploymentContextBasedVipAddresses, @@ -62,10 +61,6 @@ public class EurekaRibbonClientConfiguration { @Value("${ribbon.client.name}") private String serviceId = "client"; - protected static final String VALUE_NOT_SET = "__not__set__"; - - protected static final String DEFAULT_NAMESPACE = "ribbon"; - @Autowired(required = false) private EurekaClientConfig clientConfig; @@ -132,25 +127,8 @@ public class EurekaRibbonClientConfiguration { } } } - setProp(this.serviceId, DeploymentContextBasedVipAddresses.key(), this.serviceId); - setProp(this.serviceId, EnableZoneAffinity.key(), "true"); - } - - protected void setProp(String serviceId, String suffix, String value) { - // how to set the namespace properly? - String key = getKey(serviceId, suffix); - DynamicStringProperty property = getProperty(key); - if (property.get().equals(VALUE_NOT_SET)) { - ConfigurationManager.getConfigInstance().setProperty(key, value); - } - } - - protected DynamicStringProperty getProperty(String key) { - return DynamicPropertyFactory.getInstance().getStringProperty(key, VALUE_NOT_SET); - } - - protected String getKey(String serviceId, String suffix) { - return serviceId + "." + DEFAULT_NAMESPACE + "." + suffix; + setRibbonProperty(this.serviceId, DeploymentContextBasedVipAddresses.key(), this.serviceId); + setRibbonProperty(this.serviceId, EnableZoneAffinity.key(), "true"); } } diff --git a/spring-cloud-netflix-eureka-client/src/test/java/org/springframework/cloud/netflix/ribbon/eureka/EurekaRibbonClientConfigurationTests.java b/spring-cloud-netflix-eureka-client/src/test/java/org/springframework/cloud/netflix/ribbon/eureka/EurekaRibbonClientConfigurationTests.java index 43ebcfd1..46ac4b75 100644 --- a/spring-cloud-netflix-eureka-client/src/test/java/org/springframework/cloud/netflix/ribbon/eureka/EurekaRibbonClientConfigurationTests.java +++ b/spring-cloud-netflix-eureka-client/src/test/java/org/springframework/cloud/netflix/ribbon/eureka/EurekaRibbonClientConfigurationTests.java @@ -36,7 +36,8 @@ import com.netflix.niws.loadbalancer.DiscoveryEnabledServer; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertNotNull; import static org.junit.Assert.assertTrue; -import static org.springframework.cloud.netflix.ribbon.eureka.EurekaRibbonClientConfiguration.VALUE_NOT_SET; + +import static org.springframework.cloud.netflix.ribbon.RibbonProperyUtils.*; /** * @author Dave Syer @@ -82,12 +83,12 @@ public class EurekaRibbonClientConfigurationTests { String serviceId = "myService"; String suffix = "mySuffix"; String value = "myValue"; - DynamicStringProperty property = preprocessor.getProperty(preprocessor.getKey( + DynamicStringProperty property = getProperty(getRibbonKey( serviceId, suffix)); assertEquals("property doesn't have default value", VALUE_NOT_SET, property.get()); - preprocessor.setProp(serviceId, suffix, value); + setRibbonProperty(serviceId, suffix, value); assertEquals("property has wrong value", value, property.get()); - preprocessor.setProp(serviceId, suffix, value); + setRibbonProperty(serviceId, suffix, value); assertEquals("property has wrong value", value, property.get()); }