From 221bc47d75df331d64719d5a93a3279c07d2cb21 Mon Sep 17 00:00:00 2001 From: Spencer Gibb Date: Fri, 1 May 2020 13:09:28 -0400 Subject: [PATCH 1/9] Adds ServiceInstanceListSuppliers.java with Builder --- .../ServiceInstanceListSupplierBuilder.java | 23 +++ .../CachingServiceInstanceListSupplier.java | 13 +- ...DelegatingServiceInstanceListSupplier.java | 45 ++++++ ...ealthCheckServiceInstanceListSupplier.java | 13 +- .../core/ServiceInstanceListSuppliers.java | 131 ++++++++++++++++++ ...PreferenceServiceInstanceListSupplier.java | 13 +- .../ServiceInstanceListSuppliersTests.java | 71 ++++++++++ 7 files changed, 279 insertions(+), 30 deletions(-) create mode 100644 spring-cloud-loadbalancer/src/main/java/org/springframework/cloud/loadbalancer/annotation/configbuilder/ServiceInstanceListSupplierBuilder.java create mode 100644 spring-cloud-loadbalancer/src/main/java/org/springframework/cloud/loadbalancer/core/DelegatingServiceInstanceListSupplier.java create mode 100644 spring-cloud-loadbalancer/src/main/java/org/springframework/cloud/loadbalancer/core/ServiceInstanceListSuppliers.java create mode 100644 spring-cloud-loadbalancer/src/test/java/org/springframework/cloud/loadbalancer/core/ServiceInstanceListSuppliersTests.java diff --git a/spring-cloud-loadbalancer/src/main/java/org/springframework/cloud/loadbalancer/annotation/configbuilder/ServiceInstanceListSupplierBuilder.java b/spring-cloud-loadbalancer/src/main/java/org/springframework/cloud/loadbalancer/annotation/configbuilder/ServiceInstanceListSupplierBuilder.java new file mode 100644 index 000000000..d1411827e --- /dev/null +++ b/spring-cloud-loadbalancer/src/main/java/org/springframework/cloud/loadbalancer/annotation/configbuilder/ServiceInstanceListSupplierBuilder.java @@ -0,0 +1,23 @@ +/* + * Copyright 2013-2020 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 + * + * https://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.loadbalancer.annotation.configbuilder; + +public class ServiceInstanceListSupplierBuilder { + protected String buildNullCheckMessage(String s) { + return s + " must not be null"; + } +} diff --git a/spring-cloud-loadbalancer/src/main/java/org/springframework/cloud/loadbalancer/core/CachingServiceInstanceListSupplier.java b/spring-cloud-loadbalancer/src/main/java/org/springframework/cloud/loadbalancer/core/CachingServiceInstanceListSupplier.java index ba9692c3f..3e5a42841 100644 --- a/spring-cloud-loadbalancer/src/main/java/org/springframework/cloud/loadbalancer/core/CachingServiceInstanceListSupplier.java +++ b/spring-cloud-loadbalancer/src/main/java/org/springframework/cloud/loadbalancer/core/CachingServiceInstanceListSupplier.java @@ -37,7 +37,7 @@ * @author Olga Maciaszek-Sharma * @since 2.2.0 */ -public class CachingServiceInstanceListSupplier implements ServiceInstanceListSupplier { +public class CachingServiceInstanceListSupplier extends DelegatingServiceInstanceListSupplier { private static final Log log = LogFactory .getLog(CachingServiceInstanceListSupplier.class); @@ -48,14 +48,12 @@ public class CachingServiceInstanceListSupplier implements ServiceInstanceListSu public static final String SERVICE_INSTANCE_CACHE_NAME = CachingServiceInstanceListSupplier.class .getSimpleName() + "Cache"; - private final ServiceInstanceListSupplier delegate; - private final Flux> serviceInstances; @SuppressWarnings("unchecked") public CachingServiceInstanceListSupplier(ServiceInstanceListSupplier delegate, CacheManager cacheManager) { - this.delegate = delegate; + super(delegate); this.serviceInstances = CacheFlux.lookup(key -> { // TODO: configurable cache name Cache cache = cacheManager.getCache(SERVICE_INSTANCE_CACHE_NAME); @@ -70,7 +68,7 @@ public CachingServiceInstanceListSupplier(ServiceInstanceListSupplier delegate, return Mono.empty(); } return Flux.just(list).materialize().collectList(); - }, delegate.getServiceId()).onCacheMissResume(this.delegate) + }, delegate.getServiceId()).onCacheMissResume(delegate) .andWriteWith((key, signals) -> Flux.fromIterable(signals).dematerialize() .doOnNext(instances -> { Cache cache = cacheManager @@ -87,11 +85,6 @@ public CachingServiceInstanceListSupplier(ServiceInstanceListSupplier delegate, }).then()); } - @Override - public String getServiceId() { - return delegate.getServiceId(); - } - @Override public Flux> get() { return serviceInstances; diff --git a/spring-cloud-loadbalancer/src/main/java/org/springframework/cloud/loadbalancer/core/DelegatingServiceInstanceListSupplier.java b/spring-cloud-loadbalancer/src/main/java/org/springframework/cloud/loadbalancer/core/DelegatingServiceInstanceListSupplier.java new file mode 100644 index 000000000..89e0b59b9 --- /dev/null +++ b/spring-cloud-loadbalancer/src/main/java/org/springframework/cloud/loadbalancer/core/DelegatingServiceInstanceListSupplier.java @@ -0,0 +1,45 @@ +/* + * Copyright 2012-2019 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 + * + * https://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.loadbalancer.core; + +import java.util.function.Supplier; + +import org.springframework.cloud.client.ServiceInstance; +import org.springframework.util.Assert; + +/** + * @author Spencer Gibb + */ +public abstract class DelegatingServiceInstanceListSupplier + implements ServiceInstanceListSupplier { + + private final ServiceInstanceListSupplier delegate; + + public DelegatingServiceInstanceListSupplier(ServiceInstanceListSupplier delegate) { + Assert.notNull(delegate, "delegate may not be null"); + this.delegate = delegate; + } + + public ServiceInstanceListSupplier getDelegate() { + return this.delegate; + } + + @Override + public String getServiceId() { + return this.delegate.getServiceId(); + } +} diff --git a/spring-cloud-loadbalancer/src/main/java/org/springframework/cloud/loadbalancer/core/HealthCheckServiceInstanceListSupplier.java b/spring-cloud-loadbalancer/src/main/java/org/springframework/cloud/loadbalancer/core/HealthCheckServiceInstanceListSupplier.java index 1c5e3a9ed..c5ba66327 100644 --- a/spring-cloud-loadbalancer/src/main/java/org/springframework/cloud/loadbalancer/core/HealthCheckServiceInstanceListSupplier.java +++ b/spring-cloud-loadbalancer/src/main/java/org/springframework/cloud/loadbalancer/core/HealthCheckServiceInstanceListSupplier.java @@ -44,14 +44,12 @@ * @author Roman Matiushchenko * @since 2.2.0 */ -public class HealthCheckServiceInstanceListSupplier - implements ServiceInstanceListSupplier, InitializingBean, DisposableBean { +public class HealthCheckServiceInstanceListSupplier extends DelegatingServiceInstanceListSupplier + implements InitializingBean, DisposableBean { private static final Log LOG = LogFactory .getLog(HealthCheckServiceInstanceListSupplier.class); - private final ServiceInstanceListSupplier delegate; - private final LoadBalancerProperties.HealthCheck healthCheck; private final WebClient webClient; @@ -64,7 +62,7 @@ public class HealthCheckServiceInstanceListSupplier public HealthCheckServiceInstanceListSupplier(ServiceInstanceListSupplier delegate, LoadBalancerProperties.HealthCheck healthCheck, WebClient webClient) { - this.delegate = delegate; + super(delegate); this.healthCheck = healthCheck; defaultHealthCheckPath = healthCheck.getPath().getOrDefault("default", "/actuator/health"); @@ -121,11 +119,6 @@ protected Flux> healthCheckFlux( }).repeatWhen(restart -> restart.delayElements(healthCheck.getInterval())); } - @Override - public String getServiceId() { - return delegate.getServiceId(); - } - @Override public Flux> get() { return aliveInstancesReplay; diff --git a/spring-cloud-loadbalancer/src/main/java/org/springframework/cloud/loadbalancer/core/ServiceInstanceListSuppliers.java b/spring-cloud-loadbalancer/src/main/java/org/springframework/cloud/loadbalancer/core/ServiceInstanceListSuppliers.java new file mode 100644 index 000000000..e681a9978 --- /dev/null +++ b/spring-cloud-loadbalancer/src/main/java/org/springframework/cloud/loadbalancer/core/ServiceInstanceListSuppliers.java @@ -0,0 +1,131 @@ +/* + * Copyright 2013-2020 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 + * + * https://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.loadbalancer.core; + +import java.util.ArrayList; +import java.util.List; +import java.util.function.BiFunction; +import java.util.function.Function; + +import org.springframework.cache.CacheManager; +import org.springframework.cloud.client.discovery.DiscoveryClient; +import org.springframework.cloud.client.discovery.ReactiveDiscoveryClient; +import org.springframework.cloud.client.loadbalancer.reactive.LoadBalancerProperties; +import org.springframework.cloud.loadbalancer.config.LoadBalancerZoneConfig; +import org.springframework.context.ConfigurableApplicationContext; +import org.springframework.util.Assert; +import org.springframework.web.reactive.function.client.WebClient; + +public abstract class ServiceInstanceListSuppliers { + + private ServiceInstanceListSuppliers() { + + } + + public static Builder builder() { + return new Builder(); + } + + public interface Creator extends Function { + + } + + public interface DelegateCreator extends BiFunction { + + } + + public static class Builder { + + private Creator baseCreator; + private DelegateCreator cachingCreator; + private List creators = new ArrayList<>(); + + public Builder() { + } + + public Builder withBlockingDiscoveryClient() { + //TODO: warn if baseCreator is not null + this.baseCreator = context -> { + DiscoveryClient discoveryClient = context.getBean(DiscoveryClient.class); + + return new DiscoveryClientServiceInstanceListSupplier(discoveryClient, context.getEnvironment()); + }; + return this; + } + + public Builder withDiscoveryClient() { + //TODO: warn if baseCreator is not null + this.baseCreator = context -> { + ReactiveDiscoveryClient discoveryClient = context.getBean(ReactiveDiscoveryClient.class); + + return new DiscoveryClientServiceInstanceListSupplier(discoveryClient, context.getEnvironment()); + }; + return this; + } + + public Builder withBase(ServiceInstanceListSupplier supplier) { + this.baseCreator = context -> supplier; + return this; + } + + //TODO: overload with WebClient? + public Builder withHealthChecks() { + DelegateCreator creator = (context, delegate) -> { + LoadBalancerProperties properties = context.getBean(LoadBalancerProperties.class); + WebClient.Builder webClient = context.getBean(WebClient.Builder.class); + return new HealthCheckServiceInstanceListSupplier(delegate, properties.getHealthCheck(), webClient.build()); + }; + this.creators.add(creator); + return this; + } + + public Builder withZonePreference() { + DelegateCreator creator = (context, delegate) -> { + LoadBalancerZoneConfig zoneConfig = context.getBean(LoadBalancerZoneConfig.class); + return new ZonePreferenceServiceInstanceListSupplier(delegate, zoneConfig); + }; + this.creators.add(creator); + return this; + } + + public Builder withCaching() { + //TODO: warn if baseCreator is not null + this.cachingCreator = (context, delegate) -> { + CacheManager cacheManager = context.getBean(CacheManager.class); + + return new CachingServiceInstanceListSupplier(delegate, cacheManager); + }; + return this; + } + + public ServiceInstanceListSupplier build(ConfigurableApplicationContext context) { + Assert.notNull(baseCreator, "A baseCreator must not be null"); + + ServiceInstanceListSupplier supplier = baseCreator.apply(context); + + for (DelegateCreator creator : creators) { + supplier = creator.apply(context, supplier); + } + + if (this.cachingCreator != null) { + supplier = this.cachingCreator.apply(context, supplier); + } + return supplier; + } + + } +} diff --git a/spring-cloud-loadbalancer/src/main/java/org/springframework/cloud/loadbalancer/core/ZonePreferenceServiceInstanceListSupplier.java b/spring-cloud-loadbalancer/src/main/java/org/springframework/cloud/loadbalancer/core/ZonePreferenceServiceInstanceListSupplier.java index 540334714..76c6bbff0 100644 --- a/spring-cloud-loadbalancer/src/main/java/org/springframework/cloud/loadbalancer/core/ZonePreferenceServiceInstanceListSupplier.java +++ b/spring-cloud-loadbalancer/src/main/java/org/springframework/cloud/loadbalancer/core/ZonePreferenceServiceInstanceListSupplier.java @@ -36,30 +36,23 @@ * @since 2.2.1 */ public class ZonePreferenceServiceInstanceListSupplier - implements ServiceInstanceListSupplier { + extends DelegatingServiceInstanceListSupplier { private final String ZONE = "zone"; - private final ServiceInstanceListSupplier delegate; - private final LoadBalancerZoneConfig zoneConfig; private String zone; public ZonePreferenceServiceInstanceListSupplier(ServiceInstanceListSupplier delegate, LoadBalancerZoneConfig zoneConfig) { - this.delegate = delegate; + super(delegate); this.zoneConfig = zoneConfig; } - @Override - public String getServiceId() { - return delegate.getServiceId(); - } - @Override public Flux> get() { - return delegate.get().map(this::filteredByZone); + return getDelegate().get().map(this::filteredByZone); } private List filteredByZone(List serviceInstances) { diff --git a/spring-cloud-loadbalancer/src/test/java/org/springframework/cloud/loadbalancer/core/ServiceInstanceListSuppliersTests.java b/spring-cloud-loadbalancer/src/test/java/org/springframework/cloud/loadbalancer/core/ServiceInstanceListSuppliersTests.java new file mode 100644 index 000000000..e9af1ec79 --- /dev/null +++ b/spring-cloud-loadbalancer/src/test/java/org/springframework/cloud/loadbalancer/core/ServiceInstanceListSuppliersTests.java @@ -0,0 +1,71 @@ +/* + * Copyright 2013-2020 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 + * + * https://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.loadbalancer.core; + +import org.junit.Test; + +import org.springframework.boot.test.context.runner.ApplicationContextRunner; +import org.springframework.cache.CacheManager; +import org.springframework.cloud.client.discovery.ReactiveDiscoveryClient; +import org.springframework.cloud.client.loadbalancer.reactive.LoadBalancerProperties; +import org.springframework.context.annotation.Bean; +import org.springframework.web.reactive.function.client.WebClient; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.mockito.Mockito.mock; + +public class ServiceInstanceListSuppliersTests { + + @Test + public void testBuilder() { + new ApplicationContextRunner().withUserConfiguration(TestConfig.class).run(context -> { + ServiceInstanceListSupplier supplier = ServiceInstanceListSuppliers.builder() + .withDiscoveryClient() + .withHealthChecks() + .withCaching() + .build(context); + assertThat(supplier).isInstanceOf(CachingServiceInstanceListSupplier.class); + DelegatingServiceInstanceListSupplier delegating = (DelegatingServiceInstanceListSupplier) supplier; + assertThat(delegating.getDelegate()).isInstanceOf(HealthCheckServiceInstanceListSupplier.class); + delegating = (DelegatingServiceInstanceListSupplier) delegating.getDelegate(); + assertThat(delegating.getDelegate()).isInstanceOf(DiscoveryClientServiceInstanceListSupplier.class); + }); + } + + private static class TestConfig { + + @Bean + public ReactiveDiscoveryClient reactiveDiscoveryClient() { + return mock(ReactiveDiscoveryClient.class); + } + + @Bean + public LoadBalancerProperties loadBalancerProperties() { + return new LoadBalancerProperties(); + } + + @Bean + public CacheManager cacheManager() { + return mock(CacheManager.class); + } + + @Bean + public WebClient.Builder webClientBuilder() { + return WebClient.builder(); + } + } +} From 0976d8067072ae9828be03eb60914edd54c6b3c4 Mon Sep 17 00:00:00 2001 From: Olga Maciaszek-Sharma Date: Mon, 4 May 2020 13:40:12 +0200 Subject: [PATCH 2/9] Implement TODOs. --- ...DelegatingServiceInstanceListSupplier.java | 3 -- .../core/ServiceInstanceListSuppliers.java | 49 ++++++++++++++----- 2 files changed, 38 insertions(+), 14 deletions(-) diff --git a/spring-cloud-loadbalancer/src/main/java/org/springframework/cloud/loadbalancer/core/DelegatingServiceInstanceListSupplier.java b/spring-cloud-loadbalancer/src/main/java/org/springframework/cloud/loadbalancer/core/DelegatingServiceInstanceListSupplier.java index 89e0b59b9..92235d662 100644 --- a/spring-cloud-loadbalancer/src/main/java/org/springframework/cloud/loadbalancer/core/DelegatingServiceInstanceListSupplier.java +++ b/spring-cloud-loadbalancer/src/main/java/org/springframework/cloud/loadbalancer/core/DelegatingServiceInstanceListSupplier.java @@ -16,9 +16,6 @@ package org.springframework.cloud.loadbalancer.core; -import java.util.function.Supplier; - -import org.springframework.cloud.client.ServiceInstance; import org.springframework.util.Assert; /** diff --git a/spring-cloud-loadbalancer/src/main/java/org/springframework/cloud/loadbalancer/core/ServiceInstanceListSuppliers.java b/spring-cloud-loadbalancer/src/main/java/org/springframework/cloud/loadbalancer/core/ServiceInstanceListSuppliers.java index e681a9978..550f418c0 100644 --- a/spring-cloud-loadbalancer/src/main/java/org/springframework/cloud/loadbalancer/core/ServiceInstanceListSuppliers.java +++ b/spring-cloud-loadbalancer/src/main/java/org/springframework/cloud/loadbalancer/core/ServiceInstanceListSuppliers.java @@ -21,6 +21,9 @@ import java.util.function.BiFunction; import java.util.function.Function; +import org.apache.commons.logging.Log; +import org.apache.commons.logging.LogFactory; + import org.springframework.cache.CacheManager; import org.springframework.cloud.client.discovery.DiscoveryClient; import org.springframework.cloud.client.discovery.ReactiveDiscoveryClient; @@ -50,29 +53,38 @@ public interface DelegateCreator extends BiFunction creators = new ArrayList<>(); + private final List creators = new ArrayList<>(); public Builder() { } public Builder withBlockingDiscoveryClient() { - //TODO: warn if baseCreator is not null + if (baseCreator != null && LOG.isWarnEnabled()) { + LOG.warn("Overriding a previously set baseCreator with a blocking DiscoveryClient baseCreator."); + } this.baseCreator = context -> { DiscoveryClient discoveryClient = context.getBean(DiscoveryClient.class); - return new DiscoveryClientServiceInstanceListSupplier(discoveryClient, context.getEnvironment()); + return new DiscoveryClientServiceInstanceListSupplier(discoveryClient, context + .getEnvironment()); }; return this; } public Builder withDiscoveryClient() { - //TODO: warn if baseCreator is not null + if (baseCreator != null && LOG.isWarnEnabled()) { + LOG.warn("Overriding a previously set baseCreator with a ReactiveDiscoveryClient baseCreator."); + } this.baseCreator = context -> { - ReactiveDiscoveryClient discoveryClient = context.getBean(ReactiveDiscoveryClient.class); + ReactiveDiscoveryClient discoveryClient = context + .getBean(ReactiveDiscoveryClient.class); - return new DiscoveryClientServiceInstanceListSupplier(discoveryClient, context.getEnvironment()); + return new DiscoveryClientServiceInstanceListSupplier(discoveryClient, context + .getEnvironment()); }; return this; } @@ -82,12 +94,24 @@ public Builder withBase(ServiceInstanceListSupplier supplier) { return this; } - //TODO: overload with WebClient? public Builder withHealthChecks() { DelegateCreator creator = (context, delegate) -> { - LoadBalancerProperties properties = context.getBean(LoadBalancerProperties.class); + LoadBalancerProperties properties = context + .getBean(LoadBalancerProperties.class); WebClient.Builder webClient = context.getBean(WebClient.Builder.class); - return new HealthCheckServiceInstanceListSupplier(delegate, properties.getHealthCheck(), webClient.build()); + return new HealthCheckServiceInstanceListSupplier(delegate, properties + .getHealthCheck(), webClient.build()); + }; + this.creators.add(creator); + return this; + } + + public Builder withHealthChecks(WebClient webClient) { + DelegateCreator creator = (context, delegate) -> { + LoadBalancerProperties properties = context + .getBean(LoadBalancerProperties.class); + return new HealthCheckServiceInstanceListSupplier(delegate, properties + .getHealthCheck(), webClient); }; this.creators.add(creator); return this; @@ -95,7 +119,8 @@ public Builder withHealthChecks() { public Builder withZonePreference() { DelegateCreator creator = (context, delegate) -> { - LoadBalancerZoneConfig zoneConfig = context.getBean(LoadBalancerZoneConfig.class); + LoadBalancerZoneConfig zoneConfig = context + .getBean(LoadBalancerZoneConfig.class); return new ZonePreferenceServiceInstanceListSupplier(delegate, zoneConfig); }; this.creators.add(creator); @@ -103,7 +128,9 @@ public Builder withZonePreference() { } public Builder withCaching() { - //TODO: warn if baseCreator is not null + if (cachingCreator != null && LOG.isWarnEnabled()) { + LOG.warn("Overriding a previously set cachingCreator with a CachingServiceInstanceListSupplier-based cachingCreator."); + } this.cachingCreator = (context, delegate) -> { CacheManager cacheManager = context.getBean(CacheManager.class); From a321723bd7bb6a5cd9241fefed88031f2c735ac5 Mon Sep 17 00:00:00 2001 From: Olga Maciaszek-Sharma Date: Mon, 4 May 2020 14:06:08 +0200 Subject: [PATCH 3/9] Add javadocs. --- ...DelegatingServiceInstanceListSupplier.java | 1 + .../core/ServiceInstanceListSuppliers.java | 60 +++++++++++++++++++ 2 files changed, 61 insertions(+) diff --git a/spring-cloud-loadbalancer/src/main/java/org/springframework/cloud/loadbalancer/core/DelegatingServiceInstanceListSupplier.java b/spring-cloud-loadbalancer/src/main/java/org/springframework/cloud/loadbalancer/core/DelegatingServiceInstanceListSupplier.java index 92235d662..e93ba8538 100644 --- a/spring-cloud-loadbalancer/src/main/java/org/springframework/cloud/loadbalancer/core/DelegatingServiceInstanceListSupplier.java +++ b/spring-cloud-loadbalancer/src/main/java/org/springframework/cloud/loadbalancer/core/DelegatingServiceInstanceListSupplier.java @@ -19,6 +19,7 @@ import org.springframework.util.Assert; /** + * Represents a {@link ServiceInstanceListSupplier} that uses a delegate {@link ServiceInstanceListSupplier} instance underneath. * @author Spencer Gibb */ public abstract class DelegatingServiceInstanceListSupplier diff --git a/spring-cloud-loadbalancer/src/main/java/org/springframework/cloud/loadbalancer/core/ServiceInstanceListSuppliers.java b/spring-cloud-loadbalancer/src/main/java/org/springframework/cloud/loadbalancer/core/ServiceInstanceListSuppliers.java index 550f418c0..4200049d6 100644 --- a/spring-cloud-loadbalancer/src/main/java/org/springframework/cloud/loadbalancer/core/ServiceInstanceListSuppliers.java +++ b/spring-cloud-loadbalancer/src/main/java/org/springframework/cloud/loadbalancer/core/ServiceInstanceListSuppliers.java @@ -33,6 +33,13 @@ import org.springframework.util.Assert; import org.springframework.web.reactive.function.client.WebClient; +/** + * A utility class providing a {@link Builder} for creating a {@link ServiceInstanceListSupplier} + * hierarchy to be used in {@link ReactorLoadBalancer} configuration. + * + * @author Spencer Gibb + * @author Olga Maciaszek-Sharma + */ public abstract class ServiceInstanceListSuppliers { private ServiceInstanceListSuppliers() { @@ -43,14 +50,25 @@ public static Builder builder() { return new Builder(); } + /** + * Allows creating a {@link ServiceInstanceListSupplier} instance based on provided {@link ConfigurableApplicationContext}. + */ public interface Creator extends Function { } + /** + * Allows creating a {@link ServiceInstanceListSupplier} instance based on provided {@link ConfigurableApplicationContext} + * and another {@link ServiceInstanceListSupplier} instance that will be used as a delegate. + */ public interface DelegateCreator extends BiFunction { } + /** + * A builder for creating a {@link ServiceInstanceListSupplier} hierarchy to be used in + * {@link ReactorLoadBalancer} configuration. + */ public static class Builder { private static final Log LOG = LogFactory.getLog(Builder.class); @@ -62,6 +80,11 @@ public static class Builder { public Builder() { } + /** + * Sets a blocking {@link DiscoveryClient}-based {@link DiscoveryClientServiceInstanceListSupplier} as a base {@link ServiceInstanceListSupplier} + * in the hierarchy. + * @return the {@link Builder} object + */ public Builder withBlockingDiscoveryClient() { if (baseCreator != null && LOG.isWarnEnabled()) { LOG.warn("Overriding a previously set baseCreator with a blocking DiscoveryClient baseCreator."); @@ -75,6 +98,11 @@ public Builder withBlockingDiscoveryClient() { return this; } + /** + * Sets a {@link ReactiveDiscoveryClient}-based {@link DiscoveryClientServiceInstanceListSupplier} as a base {@link ServiceInstanceListSupplier} + * in the hierarchy. + * @return the {@link Builder} object + */ public Builder withDiscoveryClient() { if (baseCreator != null && LOG.isWarnEnabled()) { LOG.warn("Overriding a previously set baseCreator with a ReactiveDiscoveryClient baseCreator."); @@ -89,11 +117,22 @@ public Builder withDiscoveryClient() { return this; } + /** + * Sets a user-provided {@link ServiceInstanceListSupplier} as a base + * {@link ServiceInstanceListSupplier} in the hierarchy. + * @param supplier a user-provided {@link ServiceInstanceListSupplier} instance + * @return the {@link Builder} object + */ public Builder withBase(ServiceInstanceListSupplier supplier) { this.baseCreator = context -> supplier; return this; } + /** + * Adds a {@link HealthCheckServiceInstanceListSupplier} to the + * {@link ServiceInstanceListSupplier} hierarchy. + * @return the {@link Builder} object + */ public Builder withHealthChecks() { DelegateCreator creator = (context, delegate) -> { LoadBalancerProperties properties = context @@ -106,6 +145,12 @@ public Builder withHealthChecks() { return this; } + /** + * Adds a {@link HealthCheckServiceInstanceListSupplier} that uses user-provided {@link WebClient} instance + * to the {@link ServiceInstanceListSupplier} hierarchy. + * @param webClient a user-provided {@link WebClient} instance + * @return the {@link Builder} object + */ public Builder withHealthChecks(WebClient webClient) { DelegateCreator creator = (context, delegate) -> { LoadBalancerProperties properties = context @@ -117,6 +162,11 @@ public Builder withHealthChecks(WebClient webClient) { return this; } + /** + * Adds a {@link ZonePreferenceServiceInstanceListSupplier} to the + * {@link ServiceInstanceListSupplier} hierarchy. + * @return the {@link Builder} object + */ public Builder withZonePreference() { DelegateCreator creator = (context, delegate) -> { LoadBalancerZoneConfig zoneConfig = context @@ -127,6 +177,11 @@ public Builder withZonePreference() { return this; } + /** + * Wraps created {@link ServiceInstanceListSupplier} hierarchy with a {@link CachingServiceInstanceListSupplier} + * instance to provide a caching mechanism for service instances. + * @return the {@link Builder} object + */ public Builder withCaching() { if (cachingCreator != null && LOG.isWarnEnabled()) { LOG.warn("Overriding a previously set cachingCreator with a CachingServiceInstanceListSupplier-based cachingCreator."); @@ -139,6 +194,11 @@ public Builder withCaching() { return this; } + /** + * Builds the {@link ServiceInstanceListSupplier} hierarchy. + * @param context application context + * @return a {@link ServiceInstanceListSupplier} instance on top of the delegate hierarchy + */ public ServiceInstanceListSupplier build(ConfigurableApplicationContext context) { Assert.notNull(baseCreator, "A baseCreator must not be null"); From 06c43a2815e4aa267eea2b537a5a5d20c70de972 Mon Sep 17 00:00:00 2001 From: Olga Maciaszek-Sharma Date: Mon, 4 May 2020 14:07:28 +0200 Subject: [PATCH 4/9] Remove unused type. --- .../ServiceInstanceListSupplierBuilder.java | 23 ------------------- 1 file changed, 23 deletions(-) delete mode 100644 spring-cloud-loadbalancer/src/main/java/org/springframework/cloud/loadbalancer/annotation/configbuilder/ServiceInstanceListSupplierBuilder.java diff --git a/spring-cloud-loadbalancer/src/main/java/org/springframework/cloud/loadbalancer/annotation/configbuilder/ServiceInstanceListSupplierBuilder.java b/spring-cloud-loadbalancer/src/main/java/org/springframework/cloud/loadbalancer/annotation/configbuilder/ServiceInstanceListSupplierBuilder.java deleted file mode 100644 index d1411827e..000000000 --- a/spring-cloud-loadbalancer/src/main/java/org/springframework/cloud/loadbalancer/annotation/configbuilder/ServiceInstanceListSupplierBuilder.java +++ /dev/null @@ -1,23 +0,0 @@ -/* - * Copyright 2013-2020 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 - * - * https://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.loadbalancer.annotation.configbuilder; - -public class ServiceInstanceListSupplierBuilder { - protected String buildNullCheckMessage(String s) { - return s + " must not be null"; - } -} From 338dcfe770d902da89936c236a1b3668e1a27e0f Mon Sep 17 00:00:00 2001 From: Olga Maciaszek-Sharma Date: Mon, 4 May 2020 14:33:41 +0200 Subject: [PATCH 5/9] Add test. --- .../ServiceInstanceListSuppliersTests.java | 25 +++++++++++++++++-- 1 file changed, 23 insertions(+), 2 deletions(-) diff --git a/spring-cloud-loadbalancer/src/test/java/org/springframework/cloud/loadbalancer/core/ServiceInstanceListSuppliersTests.java b/spring-cloud-loadbalancer/src/test/java/org/springframework/cloud/loadbalancer/core/ServiceInstanceListSuppliersTests.java index e9af1ec79..995ca98d8 100644 --- a/spring-cloud-loadbalancer/src/test/java/org/springframework/cloud/loadbalancer/core/ServiceInstanceListSuppliersTests.java +++ b/spring-cloud-loadbalancer/src/test/java/org/springframework/cloud/loadbalancer/core/ServiceInstanceListSuppliersTests.java @@ -26,6 +26,7 @@ import org.springframework.web.reactive.function.client.WebClient; import static org.assertj.core.api.Assertions.assertThat; +import static org.junit.Assert.fail; import static org.mockito.Mockito.mock; public class ServiceInstanceListSuppliersTests { @@ -40,12 +41,32 @@ public void testBuilder() { .build(context); assertThat(supplier).isInstanceOf(CachingServiceInstanceListSupplier.class); DelegatingServiceInstanceListSupplier delegating = (DelegatingServiceInstanceListSupplier) supplier; - assertThat(delegating.getDelegate()).isInstanceOf(HealthCheckServiceInstanceListSupplier.class); + assertThat(delegating.getDelegate()) + .isInstanceOf(HealthCheckServiceInstanceListSupplier.class); delegating = (DelegatingServiceInstanceListSupplier) delegating.getDelegate(); - assertThat(delegating.getDelegate()).isInstanceOf(DiscoveryClientServiceInstanceListSupplier.class); + assertThat(delegating.getDelegate()) + .isInstanceOf(DiscoveryClientServiceInstanceListSupplier.class); }); } + @Test + public void testIllegalArgumentExceptionThrownWhenBaseBuilderNull() { + new ApplicationContextRunner().withUserConfiguration(TestConfig.class) + .run(context -> { + try { + ServiceInstanceListSuppliers.builder() + .withHealthChecks() + .build(context); + fail("Should have thrown exception."); + } + catch (Exception exception) { + assertThat(exception) + .isInstanceOf(IllegalArgumentException.class); + } + + }); + } + private static class TestConfig { @Bean From 4e5df21ff762334c664a0f40f5b7ca7ef537dfcb Mon Sep 17 00:00:00 2001 From: Olga Maciaszek-Sharma Date: Mon, 4 May 2020 17:44:07 +0200 Subject: [PATCH 6/9] Safer caching config: resolve LoadBalancerCacheManager lazily. Return delegate if LoadBalancerCacheManager not available. --- .../core/ServiceInstanceListSuppliers.java | 20 ++++- .../ServiceInstanceListSuppliersTests.java | 73 +++++++++++++------ 2 files changed, 67 insertions(+), 26 deletions(-) diff --git a/spring-cloud-loadbalancer/src/main/java/org/springframework/cloud/loadbalancer/core/ServiceInstanceListSuppliers.java b/spring-cloud-loadbalancer/src/main/java/org/springframework/cloud/loadbalancer/core/ServiceInstanceListSuppliers.java index 4200049d6..776168a63 100644 --- a/spring-cloud-loadbalancer/src/main/java/org/springframework/cloud/loadbalancer/core/ServiceInstanceListSuppliers.java +++ b/spring-cloud-loadbalancer/src/main/java/org/springframework/cloud/loadbalancer/core/ServiceInstanceListSuppliers.java @@ -24,10 +24,12 @@ import org.apache.commons.logging.Log; import org.apache.commons.logging.LogFactory; +import org.springframework.beans.factory.ObjectProvider; import org.springframework.cache.CacheManager; import org.springframework.cloud.client.discovery.DiscoveryClient; import org.springframework.cloud.client.discovery.ReactiveDiscoveryClient; import org.springframework.cloud.client.loadbalancer.reactive.LoadBalancerProperties; +import org.springframework.cloud.loadbalancer.cache.LoadBalancerCacheManager; import org.springframework.cloud.loadbalancer.config.LoadBalancerZoneConfig; import org.springframework.context.ConfigurableApplicationContext; import org.springframework.util.Assert; @@ -178,8 +180,11 @@ public Builder withZonePreference() { } /** - * Wraps created {@link ServiceInstanceListSupplier} hierarchy with a {@link CachingServiceInstanceListSupplier} + * If {@link LoadBalancerCacheManager} is available in the context, wraps created {@link ServiceInstanceListSupplier} + * hierarchy with a {@link CachingServiceInstanceListSupplier} * instance to provide a caching mechanism for service instances. + * Uses {@link ObjectProvider} to lazily resolve {@link CacheManager}. + * * @return the {@link Builder} object */ public Builder withCaching() { @@ -187,9 +192,16 @@ public Builder withCaching() { LOG.warn("Overriding a previously set cachingCreator with a CachingServiceInstanceListSupplier-based cachingCreator."); } this.cachingCreator = (context, delegate) -> { - CacheManager cacheManager = context.getBean(CacheManager.class); - - return new CachingServiceInstanceListSupplier(delegate, cacheManager); + ObjectProvider cacheManagerProvider = context + .getBeanProvider(LoadBalancerCacheManager.class); + if (cacheManagerProvider.getIfAvailable() != null) { + return new CachingServiceInstanceListSupplier(delegate, + cacheManagerProvider.getIfAvailable()); + } + if(LOG.isWarnEnabled()){ + LOG.warn("LoadBalancerCacheManager not available, returning delegate without caching."); + } + return delegate; }; return this; } diff --git a/spring-cloud-loadbalancer/src/test/java/org/springframework/cloud/loadbalancer/core/ServiceInstanceListSuppliersTests.java b/spring-cloud-loadbalancer/src/test/java/org/springframework/cloud/loadbalancer/core/ServiceInstanceListSuppliersTests.java index 995ca98d8..3be5e19be 100644 --- a/spring-cloud-loadbalancer/src/test/java/org/springframework/cloud/loadbalancer/core/ServiceInstanceListSuppliersTests.java +++ b/spring-cloud-loadbalancer/src/test/java/org/springframework/cloud/loadbalancer/core/ServiceInstanceListSuppliersTests.java @@ -19,10 +19,11 @@ import org.junit.Test; import org.springframework.boot.test.context.runner.ApplicationContextRunner; -import org.springframework.cache.CacheManager; import org.springframework.cloud.client.discovery.ReactiveDiscoveryClient; import org.springframework.cloud.client.loadbalancer.reactive.LoadBalancerProperties; +import org.springframework.cloud.loadbalancer.cache.LoadBalancerCacheManager; import org.springframework.context.annotation.Bean; +import org.springframework.context.annotation.Import; import org.springframework.web.reactive.function.client.WebClient; import static org.assertj.core.api.Assertions.assertThat; @@ -33,25 +34,29 @@ public class ServiceInstanceListSuppliersTests { @Test public void testBuilder() { - new ApplicationContextRunner().withUserConfiguration(TestConfig.class).run(context -> { - ServiceInstanceListSupplier supplier = ServiceInstanceListSuppliers.builder() - .withDiscoveryClient() - .withHealthChecks() - .withCaching() - .build(context); - assertThat(supplier).isInstanceOf(CachingServiceInstanceListSupplier.class); - DelegatingServiceInstanceListSupplier delegating = (DelegatingServiceInstanceListSupplier) supplier; - assertThat(delegating.getDelegate()) - .isInstanceOf(HealthCheckServiceInstanceListSupplier.class); - delegating = (DelegatingServiceInstanceListSupplier) delegating.getDelegate(); - assertThat(delegating.getDelegate()) - .isInstanceOf(DiscoveryClientServiceInstanceListSupplier.class); - }); + new ApplicationContextRunner().withUserConfiguration(CacheTestConfig.class) + .run(context -> { + ServiceInstanceListSupplier supplier = ServiceInstanceListSuppliers + .builder() + .withDiscoveryClient() + .withHealthChecks() + .withCaching() + .build(context); + assertThat(supplier) + .isInstanceOf(CachingServiceInstanceListSupplier.class); + DelegatingServiceInstanceListSupplier delegating = (DelegatingServiceInstanceListSupplier) supplier; + assertThat(delegating.getDelegate()) + .isInstanceOf(HealthCheckServiceInstanceListSupplier.class); + delegating = (DelegatingServiceInstanceListSupplier) delegating + .getDelegate(); + assertThat(delegating.getDelegate()) + .isInstanceOf(DiscoveryClientServiceInstanceListSupplier.class); + }); } @Test public void testIllegalArgumentExceptionThrownWhenBaseBuilderNull() { - new ApplicationContextRunner().withUserConfiguration(TestConfig.class) + new ApplicationContextRunner().withUserConfiguration(CacheTestConfig.class) .run(context -> { try { ServiceInstanceListSuppliers.builder() @@ -67,7 +72,36 @@ public void testIllegalArgumentExceptionThrownWhenBaseBuilderNull() { }); } - private static class TestConfig { + @Test + public void testDelegateReturnedIfLoadBalancerCacheManagerNotAvailable() { + new ApplicationContextRunner().withUserConfiguration(BaseTestConfig.class) + .run(context -> { + ServiceInstanceListSupplier supplier = ServiceInstanceListSuppliers + .builder() + .withDiscoveryClient() + .withHealthChecks() + .withCaching() + .build(context); + assertThat(supplier) + .isNotInstanceOf(CachingServiceInstanceListSupplier.class); + assertThat(supplier) + .isInstanceOf(HealthCheckServiceInstanceListSupplier.class); + DelegatingServiceInstanceListSupplier delegating = (DelegatingServiceInstanceListSupplier) supplier; + assertThat(delegating.getDelegate()) + .isInstanceOf(DiscoveryClientServiceInstanceListSupplier.class); + }); + } + + @Import(BaseTestConfig.class) + private static class CacheTestConfig { + + @Bean + public LoadBalancerCacheManager cacheManager() { + return mock(LoadBalancerCacheManager.class); + } + } + + private static class BaseTestConfig { @Bean public ReactiveDiscoveryClient reactiveDiscoveryClient() { @@ -79,11 +113,6 @@ public LoadBalancerProperties loadBalancerProperties() { return new LoadBalancerProperties(); } - @Bean - public CacheManager cacheManager() { - return mock(CacheManager.class); - } - @Bean public WebClient.Builder webClientBuilder() { return WebClient.builder(); From e2843a14aaafa89c0016ea3842354ef8b0d9ec30 Mon Sep 17 00:00:00 2001 From: Olga Maciaszek-Sharma Date: Mon, 4 May 2020 17:57:44 +0200 Subject: [PATCH 7/9] Switch to using builder in LoadBalancerClientConfiguration. --- .../LoadBalancerClientConfiguration.java | 32 +++++-------------- 1 file changed, 8 insertions(+), 24 deletions(-) diff --git a/spring-cloud-loadbalancer/src/main/java/org/springframework/cloud/loadbalancer/annotation/LoadBalancerClientConfiguration.java b/spring-cloud-loadbalancer/src/main/java/org/springframework/cloud/loadbalancer/annotation/LoadBalancerClientConfiguration.java index c93d65b1c..599ffd291 100644 --- a/spring-cloud-loadbalancer/src/main/java/org/springframework/cloud/loadbalancer/annotation/LoadBalancerClientConfiguration.java +++ b/spring-cloud-loadbalancer/src/main/java/org/springframework/cloud/loadbalancer/annotation/LoadBalancerClientConfiguration.java @@ -26,16 +26,16 @@ import org.springframework.cloud.client.discovery.DiscoveryClient; import org.springframework.cloud.client.discovery.ReactiveDiscoveryClient; import org.springframework.cloud.loadbalancer.cache.LoadBalancerCacheManager; -import org.springframework.cloud.loadbalancer.core.CachingServiceInstanceListSupplier; import org.springframework.cloud.loadbalancer.core.CachingServiceInstanceSupplier; -import org.springframework.cloud.loadbalancer.core.DiscoveryClientServiceInstanceListSupplier; import org.springframework.cloud.loadbalancer.core.DiscoveryClientServiceInstanceSupplier; import org.springframework.cloud.loadbalancer.core.ReactorLoadBalancer; import org.springframework.cloud.loadbalancer.core.RoundRobinLoadBalancer; import org.springframework.cloud.loadbalancer.core.ServiceInstanceListSupplier; +import org.springframework.cloud.loadbalancer.core.ServiceInstanceListSuppliers; import org.springframework.cloud.loadbalancer.core.ServiceInstanceSupplier; import org.springframework.cloud.loadbalancer.support.LoadBalancerClientFactory; import org.springframework.context.ApplicationContext; +import org.springframework.context.ConfigurableApplicationContext; import org.springframework.context.annotation.Bean; import org.springframework.context.annotation.Configuration; import org.springframework.core.annotation.Order; @@ -71,17 +71,9 @@ public static class ReactiveSupportConfiguration { @ConditionalOnBean(ReactiveDiscoveryClient.class) @ConditionalOnMissingBean public ServiceInstanceListSupplier discoveryClientServiceInstanceListSupplier( - ReactiveDiscoveryClient discoveryClient, Environment env, - ApplicationContext context) { - DiscoveryClientServiceInstanceListSupplier delegate = new DiscoveryClientServiceInstanceListSupplier( - discoveryClient, env); - ObjectProvider cacheManagerProvider = context - .getBeanProvider(LoadBalancerCacheManager.class); - if (cacheManagerProvider.getIfAvailable() != null) { - return new CachingServiceInstanceListSupplier(delegate, - cacheManagerProvider.getIfAvailable()); - } - return delegate; + ConfigurableApplicationContext context) { + return ServiceInstanceListSuppliers.builder().withDiscoveryClient() + .withCaching().build(context); } @Bean @@ -112,17 +104,9 @@ public static class BlockingSupportConfiguration { @ConditionalOnBean(DiscoveryClient.class) @ConditionalOnMissingBean public ServiceInstanceListSupplier discoveryClientServiceInstanceListSupplier( - DiscoveryClient discoveryClient, Environment env, - ApplicationContext context) { - DiscoveryClientServiceInstanceListSupplier delegate = new DiscoveryClientServiceInstanceListSupplier( - discoveryClient, env); - ObjectProvider cacheManagerProvider = context - .getBeanProvider(LoadBalancerCacheManager.class); - if (cacheManagerProvider.getIfAvailable() != null) { - return new CachingServiceInstanceListSupplier(delegate, - cacheManagerProvider.getIfAvailable()); - } - return delegate; + ConfigurableApplicationContext context) { + return ServiceInstanceListSuppliers.builder().withBlockingDiscoveryClient() + .withCaching().build(context); } @Bean From 0b4a47d405b7feff61390d16c3ee4cb322a4a8bd Mon Sep 17 00:00:00 2001 From: Olga Maciaszek-Sharma Date: Mon, 4 May 2020 17:58:23 +0200 Subject: [PATCH 8/9] Autoformatting with spring-java-format. --- .../CachingServiceInstanceListSupplier.java | 3 +- ...DelegatingServiceInstanceListSupplier.java | 5 +- ...ealthCheckServiceInstanceListSupplier.java | 3 +- .../core/ServiceInstanceListSuppliers.java | 88 +++++++++++-------- .../ServiceInstanceListSuppliersTests.java | 28 +++--- 5 files changed, 72 insertions(+), 55 deletions(-) diff --git a/spring-cloud-loadbalancer/src/main/java/org/springframework/cloud/loadbalancer/core/CachingServiceInstanceListSupplier.java b/spring-cloud-loadbalancer/src/main/java/org/springframework/cloud/loadbalancer/core/CachingServiceInstanceListSupplier.java index 3e5a42841..aecca695e 100644 --- a/spring-cloud-loadbalancer/src/main/java/org/springframework/cloud/loadbalancer/core/CachingServiceInstanceListSupplier.java +++ b/spring-cloud-loadbalancer/src/main/java/org/springframework/cloud/loadbalancer/core/CachingServiceInstanceListSupplier.java @@ -37,7 +37,8 @@ * @author Olga Maciaszek-Sharma * @since 2.2.0 */ -public class CachingServiceInstanceListSupplier extends DelegatingServiceInstanceListSupplier { +public class CachingServiceInstanceListSupplier + extends DelegatingServiceInstanceListSupplier { private static final Log log = LogFactory .getLog(CachingServiceInstanceListSupplier.class); diff --git a/spring-cloud-loadbalancer/src/main/java/org/springframework/cloud/loadbalancer/core/DelegatingServiceInstanceListSupplier.java b/spring-cloud-loadbalancer/src/main/java/org/springframework/cloud/loadbalancer/core/DelegatingServiceInstanceListSupplier.java index e93ba8538..7cc9d53f8 100644 --- a/spring-cloud-loadbalancer/src/main/java/org/springframework/cloud/loadbalancer/core/DelegatingServiceInstanceListSupplier.java +++ b/spring-cloud-loadbalancer/src/main/java/org/springframework/cloud/loadbalancer/core/DelegatingServiceInstanceListSupplier.java @@ -19,7 +19,9 @@ import org.springframework.util.Assert; /** - * Represents a {@link ServiceInstanceListSupplier} that uses a delegate {@link ServiceInstanceListSupplier} instance underneath. + * Represents a {@link ServiceInstanceListSupplier} that uses a delegate + * {@link ServiceInstanceListSupplier} instance underneath. + * * @author Spencer Gibb */ public abstract class DelegatingServiceInstanceListSupplier @@ -40,4 +42,5 @@ public ServiceInstanceListSupplier getDelegate() { public String getServiceId() { return this.delegate.getServiceId(); } + } diff --git a/spring-cloud-loadbalancer/src/main/java/org/springframework/cloud/loadbalancer/core/HealthCheckServiceInstanceListSupplier.java b/spring-cloud-loadbalancer/src/main/java/org/springframework/cloud/loadbalancer/core/HealthCheckServiceInstanceListSupplier.java index c5ba66327..85ad174b9 100644 --- a/spring-cloud-loadbalancer/src/main/java/org/springframework/cloud/loadbalancer/core/HealthCheckServiceInstanceListSupplier.java +++ b/spring-cloud-loadbalancer/src/main/java/org/springframework/cloud/loadbalancer/core/HealthCheckServiceInstanceListSupplier.java @@ -44,7 +44,8 @@ * @author Roman Matiushchenko * @since 2.2.0 */ -public class HealthCheckServiceInstanceListSupplier extends DelegatingServiceInstanceListSupplier +public class HealthCheckServiceInstanceListSupplier + extends DelegatingServiceInstanceListSupplier implements InitializingBean, DisposableBean { private static final Log LOG = LogFactory diff --git a/spring-cloud-loadbalancer/src/main/java/org/springframework/cloud/loadbalancer/core/ServiceInstanceListSuppliers.java b/spring-cloud-loadbalancer/src/main/java/org/springframework/cloud/loadbalancer/core/ServiceInstanceListSuppliers.java index 776168a63..690a53eed 100644 --- a/spring-cloud-loadbalancer/src/main/java/org/springframework/cloud/loadbalancer/core/ServiceInstanceListSuppliers.java +++ b/spring-cloud-loadbalancer/src/main/java/org/springframework/cloud/loadbalancer/core/ServiceInstanceListSuppliers.java @@ -25,7 +25,6 @@ import org.apache.commons.logging.LogFactory; import org.springframework.beans.factory.ObjectProvider; -import org.springframework.cache.CacheManager; import org.springframework.cloud.client.discovery.DiscoveryClient; import org.springframework.cloud.client.discovery.ReactiveDiscoveryClient; import org.springframework.cloud.client.loadbalancer.reactive.LoadBalancerProperties; @@ -36,8 +35,9 @@ import org.springframework.web.reactive.function.client.WebClient; /** - * A utility class providing a {@link Builder} for creating a {@link ServiceInstanceListSupplier} - * hierarchy to be used in {@link ReactorLoadBalancer} configuration. + * A utility class providing a {@link Builder} for creating a + * {@link ServiceInstanceListSupplier} hierarchy to be used in {@link ReactorLoadBalancer} + * configuration. * * @author Spencer Gibb * @author Olga Maciaszek-Sharma @@ -53,68 +53,78 @@ public static Builder builder() { } /** - * Allows creating a {@link ServiceInstanceListSupplier} instance based on provided {@link ConfigurableApplicationContext}. + * Allows creating a {@link ServiceInstanceListSupplier} instance based on provided + * {@link ConfigurableApplicationContext}. */ - public interface Creator extends Function { + public interface Creator extends + Function { } /** - * Allows creating a {@link ServiceInstanceListSupplier} instance based on provided {@link ConfigurableApplicationContext} - * and another {@link ServiceInstanceListSupplier} instance that will be used as a delegate. + * Allows creating a {@link ServiceInstanceListSupplier} instance based on provided + * {@link ConfigurableApplicationContext} and another + * {@link ServiceInstanceListSupplier} instance that will be used as a delegate. */ - public interface DelegateCreator extends BiFunction { + public interface DelegateCreator extends + BiFunction { } /** - * A builder for creating a {@link ServiceInstanceListSupplier} hierarchy to be used in - * {@link ReactorLoadBalancer} configuration. + * A builder for creating a {@link ServiceInstanceListSupplier} hierarchy to be used + * in {@link ReactorLoadBalancer} configuration. */ public static class Builder { private static final Log LOG = LogFactory.getLog(Builder.class); private Creator baseCreator; + private DelegateCreator cachingCreator; + private final List creators = new ArrayList<>(); public Builder() { } /** - * Sets a blocking {@link DiscoveryClient}-based {@link DiscoveryClientServiceInstanceListSupplier} as a base {@link ServiceInstanceListSupplier} - * in the hierarchy. + * Sets a blocking {@link DiscoveryClient}-based + * {@link DiscoveryClientServiceInstanceListSupplier} as a base + * {@link ServiceInstanceListSupplier} in the hierarchy. * @return the {@link Builder} object */ public Builder withBlockingDiscoveryClient() { if (baseCreator != null && LOG.isWarnEnabled()) { - LOG.warn("Overriding a previously set baseCreator with a blocking DiscoveryClient baseCreator."); + LOG.warn( + "Overriding a previously set baseCreator with a blocking DiscoveryClient baseCreator."); } this.baseCreator = context -> { DiscoveryClient discoveryClient = context.getBean(DiscoveryClient.class); - return new DiscoveryClientServiceInstanceListSupplier(discoveryClient, context - .getEnvironment()); + return new DiscoveryClientServiceInstanceListSupplier(discoveryClient, + context.getEnvironment()); }; return this; } /** - * Sets a {@link ReactiveDiscoveryClient}-based {@link DiscoveryClientServiceInstanceListSupplier} as a base {@link ServiceInstanceListSupplier} - * in the hierarchy. + * Sets a {@link ReactiveDiscoveryClient}-based + * {@link DiscoveryClientServiceInstanceListSupplier} as a base + * {@link ServiceInstanceListSupplier} in the hierarchy. * @return the {@link Builder} object */ public Builder withDiscoveryClient() { if (baseCreator != null && LOG.isWarnEnabled()) { - LOG.warn("Overriding a previously set baseCreator with a ReactiveDiscoveryClient baseCreator."); + LOG.warn( + "Overriding a previously set baseCreator with a ReactiveDiscoveryClient baseCreator."); } this.baseCreator = context -> { ReactiveDiscoveryClient discoveryClient = context .getBean(ReactiveDiscoveryClient.class); - return new DiscoveryClientServiceInstanceListSupplier(discoveryClient, context - .getEnvironment()); + return new DiscoveryClientServiceInstanceListSupplier(discoveryClient, + context.getEnvironment()); }; return this; } @@ -140,16 +150,17 @@ public Builder withHealthChecks() { LoadBalancerProperties properties = context .getBean(LoadBalancerProperties.class); WebClient.Builder webClient = context.getBean(WebClient.Builder.class); - return new HealthCheckServiceInstanceListSupplier(delegate, properties - .getHealthCheck(), webClient.build()); + return new HealthCheckServiceInstanceListSupplier(delegate, + properties.getHealthCheck(), webClient.build()); }; this.creators.add(creator); return this; } /** - * Adds a {@link HealthCheckServiceInstanceListSupplier} that uses user-provided {@link WebClient} instance - * to the {@link ServiceInstanceListSupplier} hierarchy. + * Adds a {@link HealthCheckServiceInstanceListSupplier} that uses user-provided + * {@link WebClient} instance to the {@link ServiceInstanceListSupplier} + * hierarchy. * @param webClient a user-provided {@link WebClient} instance * @return the {@link Builder} object */ @@ -157,8 +168,8 @@ public Builder withHealthChecks(WebClient webClient) { DelegateCreator creator = (context, delegate) -> { LoadBalancerProperties properties = context .getBean(LoadBalancerProperties.class); - return new HealthCheckServiceInstanceListSupplier(delegate, properties - .getHealthCheck(), webClient); + return new HealthCheckServiceInstanceListSupplier(delegate, + properties.getHealthCheck(), webClient); }; this.creators.add(creator); return this; @@ -173,23 +184,25 @@ public Builder withZonePreference() { DelegateCreator creator = (context, delegate) -> { LoadBalancerZoneConfig zoneConfig = context .getBean(LoadBalancerZoneConfig.class); - return new ZonePreferenceServiceInstanceListSupplier(delegate, zoneConfig); + return new ZonePreferenceServiceInstanceListSupplier(delegate, + zoneConfig); }; this.creators.add(creator); return this; } /** - * If {@link LoadBalancerCacheManager} is available in the context, wraps created {@link ServiceInstanceListSupplier} - * hierarchy with a {@link CachingServiceInstanceListSupplier} - * instance to provide a caching mechanism for service instances. - * Uses {@link ObjectProvider} to lazily resolve {@link CacheManager}. - * + * If {@link LoadBalancerCacheManager} is available in the context, wraps created + * {@link ServiceInstanceListSupplier} hierarchy with a + * {@link CachingServiceInstanceListSupplier} instance to provide a caching + * mechanism for service instances. Uses {@link ObjectProvider} to lazily resolve + * {@link LoadBalancerCacheManager}. * @return the {@link Builder} object */ public Builder withCaching() { if (cachingCreator != null && LOG.isWarnEnabled()) { - LOG.warn("Overriding a previously set cachingCreator with a CachingServiceInstanceListSupplier-based cachingCreator."); + LOG.warn( + "Overriding a previously set cachingCreator with a CachingServiceInstanceListSupplier-based cachingCreator."); } this.cachingCreator = (context, delegate) -> { ObjectProvider cacheManagerProvider = context @@ -198,8 +211,9 @@ public Builder withCaching() { return new CachingServiceInstanceListSupplier(delegate, cacheManagerProvider.getIfAvailable()); } - if(LOG.isWarnEnabled()){ - LOG.warn("LoadBalancerCacheManager not available, returning delegate without caching."); + if (LOG.isWarnEnabled()) { + LOG.warn( + "LoadBalancerCacheManager not available, returning delegate without caching."); } return delegate; }; @@ -209,7 +223,8 @@ public Builder withCaching() { /** * Builds the {@link ServiceInstanceListSupplier} hierarchy. * @param context application context - * @return a {@link ServiceInstanceListSupplier} instance on top of the delegate hierarchy + * @return a {@link ServiceInstanceListSupplier} instance on top of the delegate + * hierarchy */ public ServiceInstanceListSupplier build(ConfigurableApplicationContext context) { Assert.notNull(baseCreator, "A baseCreator must not be null"); @@ -227,4 +242,5 @@ public ServiceInstanceListSupplier build(ConfigurableApplicationContext context) } } + } diff --git a/spring-cloud-loadbalancer/src/test/java/org/springframework/cloud/loadbalancer/core/ServiceInstanceListSuppliersTests.java b/spring-cloud-loadbalancer/src/test/java/org/springframework/cloud/loadbalancer/core/ServiceInstanceListSuppliersTests.java index 3be5e19be..39208e52d 100644 --- a/spring-cloud-loadbalancer/src/test/java/org/springframework/cloud/loadbalancer/core/ServiceInstanceListSuppliersTests.java +++ b/spring-cloud-loadbalancer/src/test/java/org/springframework/cloud/loadbalancer/core/ServiceInstanceListSuppliersTests.java @@ -37,11 +37,8 @@ public void testBuilder() { new ApplicationContextRunner().withUserConfiguration(CacheTestConfig.class) .run(context -> { ServiceInstanceListSupplier supplier = ServiceInstanceListSuppliers - .builder() - .withDiscoveryClient() - .withHealthChecks() - .withCaching() - .build(context); + .builder().withDiscoveryClient().withHealthChecks() + .withCaching().build(context); assertThat(supplier) .isInstanceOf(CachingServiceInstanceListSupplier.class); DelegatingServiceInstanceListSupplier delegating = (DelegatingServiceInstanceListSupplier) supplier; @@ -49,8 +46,8 @@ public void testBuilder() { .isInstanceOf(HealthCheckServiceInstanceListSupplier.class); delegating = (DelegatingServiceInstanceListSupplier) delegating .getDelegate(); - assertThat(delegating.getDelegate()) - .isInstanceOf(DiscoveryClientServiceInstanceListSupplier.class); + assertThat(delegating.getDelegate()).isInstanceOf( + DiscoveryClientServiceInstanceListSupplier.class); }); } @@ -59,8 +56,7 @@ public void testIllegalArgumentExceptionThrownWhenBaseBuilderNull() { new ApplicationContextRunner().withUserConfiguration(CacheTestConfig.class) .run(context -> { try { - ServiceInstanceListSuppliers.builder() - .withHealthChecks() + ServiceInstanceListSuppliers.builder().withHealthChecks() .build(context); fail("Should have thrown exception."); } @@ -77,18 +73,15 @@ public void testDelegateReturnedIfLoadBalancerCacheManagerNotAvailable() { new ApplicationContextRunner().withUserConfiguration(BaseTestConfig.class) .run(context -> { ServiceInstanceListSupplier supplier = ServiceInstanceListSuppliers - .builder() - .withDiscoveryClient() - .withHealthChecks() - .withCaching() - .build(context); + .builder().withDiscoveryClient().withHealthChecks() + .withCaching().build(context); assertThat(supplier) .isNotInstanceOf(CachingServiceInstanceListSupplier.class); assertThat(supplier) .isInstanceOf(HealthCheckServiceInstanceListSupplier.class); DelegatingServiceInstanceListSupplier delegating = (DelegatingServiceInstanceListSupplier) supplier; - assertThat(delegating.getDelegate()) - .isInstanceOf(DiscoveryClientServiceInstanceListSupplier.class); + assertThat(delegating.getDelegate()).isInstanceOf( + DiscoveryClientServiceInstanceListSupplier.class); }); } @@ -99,6 +92,7 @@ private static class CacheTestConfig { public LoadBalancerCacheManager cacheManager() { return mock(LoadBalancerCacheManager.class); } + } private static class BaseTestConfig { @@ -117,5 +111,7 @@ public LoadBalancerProperties loadBalancerProperties() { public WebClient.Builder webClientBuilder() { return WebClient.builder(); } + } + } From 72ad39b96e53f864f038eec260e03a55329f9263 Mon Sep 17 00:00:00 2001 From: Olga Maciaszek-Sharma Date: Mon, 4 May 2020 18:08:58 +0200 Subject: [PATCH 9/9] Update docs. --- .../main/asciidoc/spring-cloud-commons.adoc | 47 +++++++------------ 1 file changed, 16 insertions(+), 31 deletions(-) diff --git a/docs/src/main/asciidoc/spring-cloud-commons.adoc b/docs/src/main/asciidoc/spring-cloud-commons.adoc index 2691e5fe6..f6652c1da 100644 --- a/docs/src/main/asciidoc/spring-cloud-commons.adoc +++ b/docs/src/main/asciidoc/spring-cloud-commons.adoc @@ -917,22 +917,14 @@ public class CustomLoadBalancerConfiguration { @Bean public ServiceInstanceListSupplier discoveryClientServiceInstanceListSupplier( - ReactiveDiscoveryClient discoveryClient, Environment environment, - LoadBalancerZoneConfig zoneConfig, - ApplicationContext context) { - DiscoveryClientServiceInstanceListSupplier firstDelegate = new DiscoveryClientServiceInstanceListSupplier( - discoveryClient, environment); - ZonePreferenceServiceInstanceListSupplier delegate = new ZonePreferenceServiceInstanceListSupplier(firstDelegate, - zoneConfig); - ObjectProvider cacheManagerProvider = context - .getBeanProvider(LoadBalancerCacheManager.class); - if (cacheManagerProvider.getIfAvailable() != null) { - return new CachingServiceInstanceListSupplier(delegate, - cacheManagerProvider.getIfAvailable()); - } - return delegate; - } + ConfigurableApplicationContext context) { + return ServiceInstanceListSuppliers.builder() + .withDiscoveryClient() + .withZonePreference() + .withCaching() + .build(context); } +} ---- === Instance Health-Check for LoadBalancer @@ -966,25 +958,18 @@ public class CustomLoadBalancerConfiguration { @Bean public ServiceInstanceListSupplier discoveryClientServiceInstanceListSupplier( - ReactiveDiscoveryClient discoveryClient, Environment environment, - LoadBalancerProperties loadBalancerProperties, - ApplicationContext context, - InstanceHealthChecker healthChecker) { - DiscoveryClientServiceInstanceListSupplier firstDelegate = new DiscoveryClientServiceInstanceListSupplier( - discoveryClient, environment); - HealthCheckServiceInstanceListSupplier delegate = new HealthCheckServiceInstanceListSupplier(firstDelegate, - loadBalancerProperties, healthChecker); - ObjectProvider cacheManagerProvider = context - .getBeanProvider(LoadBalancerCacheManager.class); - if (cacheManagerProvider.getIfAvailable() != null) { - return new CachingServiceInstanceListSupplier(delegate, - cacheManagerProvider.getIfAvailable()); - } - return delegate; - } + ConfigurableApplicationContext context) { + return ServiceInstanceListSuppliers.builder() + .withDiscoveryClient() + .withHealthChecks() + .withCaching() + .build(context); + } } ---- +TIP:: In order to make working on your own LoadBalancer configuration easier, we have added some utility methods in `ServiceInstanceListSuppliers` class. + [[spring-cloud-loadbalancer-starter]] === Spring Cloud LoadBalancer Starter