Support properties configuration per load balancer client #947

Ferioney · 2021-04-22T11:06:01Z

Support properties configuration per client

Fixes #914

Ferioney · 2021-04-23T07:56:39Z

...framework/cloud/client/loadbalancer/reactive/ReactorLoadBalancerClientAutoConfiguration.java

 	}

 	@ConditionalOnMissingBean
 	@ConditionalOnProperty(value = "spring.cloud.loadbalancer.retry.enabled", havingValue = "true")


@OlgaMaciaszek @spencergibb

I see an issue with enablingLoadBalancerRetryPolicy by spring.cloud.loadbalancer.retry.enabled .

I think we can use spring.cloud.loadbalancer.retry.enabled property for enabling retry in general. And then we should check in policy (and in other beans) that retry is enabled for a specific service (it has not implemented yet).

Yes, we definitely should keep spring.cloud.loadbalancer.retry.enabled to be used as a global switch, but we could probably also be able to set spring.cloud.loadbalancer.client.clientName.retry.enabled per client - wdyt?

But I'm not sure how to implement switching by spring.cloud.loadbalancer.client.clientName.retry.enabled for reactive loadBalancer. Reactive loadBalance has two functions - with (RetryableLoadBalancerExchangeFilterFunction) and without (ReactorLoadBalancerExchangeFilterFunction) retry. and as I see there no possibility to create pipeline for a few filters (we can but we don't have an order guaranty). so I tried to make it in retry filter - RetryableLoadBalancerExchangeFilterFunction.java

That looks fine, but I'm thinking we also need a way to handle global properties; I imagine we would do so with retries, but also possibly with caching (we should allow disabling it globally - could you add this?) and possibly other functionalities in the future, so apart from the method getLoadBalancerProperties(String serviceName) in the factory, we could also have sth like getGlobalLoadBalancerProperties() (that would just return the non-client-prefixed properties - then for the major we would have to move most of the properties to the spring.cloud.loadbalancer.client.clientName - prefixed, and only leave the global ones prefixed with just spring.cloud.loadbalancer) and in cases where we want to offer a global off switch (or some other global property) we could verify first the global properties. wdyt?

I'll add getGlobalLoadBalancerProperties() method to the factory. But I don't see where we can use it.

For caching we use a different property class LoadBalancerCacheProperties.java. Change cache classes to use the new way of configuration not enough, functionality also should be changed (DefaultLoadBalancerCacheManager and CaffeineBasedLoadBalancerCacheManager). I think it should be done in a separate thread.

Taking this into account, we could actually change it and remove the getGlobalLoadBalancerProperties() while maintaining the fallback to global properties.

I think we should change the CacheManagers and properties here as well, and also possibly try to introduce this functionality for some other properties that start with spring.cloud.loadbalancer but are used in @ConditionalOnProperty - we will not be able to get this value from configuration properties at bean creation, but we could allow for an additional checkup per service within the implementation class. For example, we have:

@ConditionalOnProperty(value = "spring.cloud.loadbalancer.retry.avoid-previous-instance", havingValue = "true", matchIfMissing = true) static class AvoidPreviousInstanceEnabled { }

where the prop is used to disable this globally, but we could also allow disabling per serviceId, by adding logic to RetryAwareServiceInstanceListSupplier that verifies property per service. Could you search by spring.cloud.loadblanacer. String and verify where else it makes sense to have both global and per-service levels?

I think we can take more time with this PR if necessary, but we should introduce a complete feature where all the properties can be set per service, unless we decide that, in a given case, it does not make much sense - we should then list these exceptional properties in the docs, though.

The issue I see here, however, is that, in keeping, with this approach, we would give higher priority to per-client properties, however, in case of bean-creation-level conditionals (as in the example above) it would work opposite and this kind of inconsistency needs to be avoided.

Alternatively, I'm thinking, we could have:

spring.cloud.loadbalancer.property - global level properties; in the next major we would also leave them only for a few selected flags, things that we want to be able to switch off globally like retries and caching, flags that we check during bean creation, etc. - they would have top priority and supersede any per-client properties.

spring.cloud.loadbalancer.client.xxx.property - per-client properties, used for most things

spring.cloud.loadbalancer.client.default.property - default fallback set by the user, used when per-client property is not set.

default property set by us (from new LoadBalancerProperties())

@Ferioney @spencergibb wdyt? Let's discuss it here first and decide on it before you introduce any new code changes, @Ferioney .

Another option would be to rework the way the ServiceInstanceListSupplier beans are being created and, in place of using @ConditionalOnProperty during bean creation, use the properties bean/ factory passed in constructor and then create the bean, for example, new RetryAwareServiceInstanceListSupplier(delegate) (see usage from current code) if the flag from properties is true and just return the delegate bean if it's false. Then we would be able to always give the highest priority for spring.cloud.loadbalancer.client.xxx.property and fall back to spring.cloud.loadbalancer.property consistently for all the features. @Ferioney @spencergibb please let me know what you think about the various possible approaches/ propose others.

OlgaMaciaszek

@Ferioney thanks for submitting this. In general, this looks good. Have added some comments, though - please address. Also, please add some tests verifying specifically the new property logic. Please add information in the docs regarding this feature (in spring-cloud-commons.adoc, within the "Spring Cloud LoadBalancer" section.

...c/main/java/org/springframework/cloud/client/loadbalancer/LoadBalancerPropertiesFactory.java

...c/main/java/org/springframework/cloud/client/loadbalancer/LoadBalancerServiceProperties.java

...rc/main/java/org/springframework/cloud/client/loadbalancer/RetryLoadBalancerInterceptor.java

...ain/java/org/springframework/cloud/client/loadbalancer/reactive/LoadBalancerRetryPolicy.java

...ramework/cloud/client/loadbalancer/reactive/RetryableLoadBalancerExchangeFilterFunction.java

.../java/org/springframework/cloud/loadbalancer/blocking/client/BlockingLoadBalancerClient.java

.../org/springframework/cloud/loadbalancer/blocking/retry/BlockingLoadBalancedRetryFactory.java

...rg/springframework/cloud/loadbalancer/core/LoadBalancerServiceInstanceCookieTransformer.java

OlgaMaciaszek

@Ferioney thanks for submitting the changes. Handling global properties needs to be further discussed and potentially some implementation added there (see comments). I have also requested a change in the way the client properties are prefixed and adding more docs (also please, rebuild the project with -Pdocs flag before pushing the changes - some documentation will be regenerated).

As we are getting closer to merging these changes, there's one more thing to consider - there are other projects that use LoadBalancerProperties directly, mainly SC Gateway and SC OpenFeign, so this PR will need to be accompanied with corresponding PRs in those projects - will you have time to work on it or do you want us to handle it once this one is merged?

docs/src/main/asciidoc/spring-cloud-commons.adoc

OlgaMaciaszek · 2021-04-27T10:13:57Z

...c/main/java/org/springframework/cloud/client/loadbalancer/LoadBalancerServiceProperties.java

+@ConfigurationProperties("spring.cloud.loadbalancer.client")
+public class LoadBalancerServiceProperties {
+
+	private Map<String, LoadBalancerProperties> services = new HashMap<>();


Could you modify the implementation, so that the property is spring.cloud.loadbalancer.client.foo1.healthcheck... as opposed to spring.cloud.loadbalancer.client.services.foo1.healthcheck...?

...c/main/java/org/springframework/cloud/client/loadbalancer/LoadBalancerAutoConfiguration.java

spencergibb

Some thoughts, not totally reviewed

spencergibb · 2021-04-27T21:27:12Z

...c/main/java/org/springframework/cloud/client/loadbalancer/LoadBalancerServiceProperties.java

+@ConfigurationProperties("spring.cloud.loadbalancer.client")
+public class LoadBalancerServiceProperties {
+
+	private Map<String, LoadBalancerProperties> services = new HashMap<>();


spencergibb · 2021-04-27T21:29:39Z

...rc/main/java/org/springframework/cloud/client/loadbalancer/RetryLoadBalancerInterceptor.java

+	@Deprecated
+	private LoadBalancerProperties getLoadBalancerProperties(String serviceId) {
+		if (propertiesFactory != null) {
+			return propertiesFactory.getLoadBalancerProperties(serviceId);


I think this isn't quite right. If there isn't properties for a service, the properties object should be returned.

Factory method returns default properties when service-specific properties were not provided.

private LoadBalancerProperties getLoadBalancerServiceProperties(String serviceName) { return servicesProperties.getClient().getOrDefault(serviceName, servicesProperties.getDefaultClientProperties()); }

spencergibb · 2021-04-27T21:31:25Z

...ain/java/org/springframework/cloud/client/loadbalancer/reactive/LoadBalancerRetryPolicy.java

+	 * @param context the context for the retry operation
+	 * @return true to retry on the same service instance
+	 */
+	default boolean canRetrySameServiceInstance(String serviceId, LoadBalancerRetryContext context) {


Why couldn't LoadBalancerRetryContext have the serviceId so all these methods won't need to change

LoadBalancerRetryContext is created in ReactorLoadBalancerClientAutoConfiguration. At that time we don't have information about service names. So I decided to add new methods.

Yes, we don't have it there.

Ferioney · 2021-04-30T14:02:51Z

@OlgaMaciaszek thank you for your answer.
yes, it should be done in SC Gateway and SC OpenFeign. I can try to help with it.

OlgaMaciaszek · 2021-05-06T15:07:56Z

@Ferioney upon consideration, I think @spencergibb idea here makes sense. It would also resolve the global properties issue - we would automatically use per-client, but if there's none then use the global one OOTB - we can then remove the getGlobalProperties method and add information to the docs, that per-client properties will override global properties, but we will fall back to global if per-client is not set.

EDIT: It can, however, cuase inconsistencies regarding properties hierarchy (see #947 (comment)). Let's discuss it there, before adding any further code changes.

OlgaMaciaszek

@Ferioney, thanks for the updates I have added some comments regarding variable renaming, however, the real issue seems to be creating a consistent properties hierarchy for all the spring.cloud.loadbalancer-prefixed properties, as discussed here. Let's first reach a conclusion of that discussion before adding the code changes.

OlgaMaciaszek · 2021-05-06T14:39:44Z

...c/main/java/org/springframework/cloud/client/loadbalancer/LoadBalancerPropertiesFactory.java

+
+	private final LoadBalancerProperties globalProperties;
+
+	private final LoadBalancerClientProperties servicesProperties;


Let's rename the variable to clientProperties.

OlgaMaciaszek · 2021-05-06T14:40:43Z

...c/main/java/org/springframework/cloud/client/loadbalancer/LoadBalancerPropertiesFactory.java

+
+	private final LoadBalancerClientProperties servicesProperties;
+
+	private final boolean isServiceProperties;


Let's rename to useClientProperties

OlgaMaciaszek · 2021-05-06T14:42:33Z

...c/main/java/org/springframework/cloud/client/loadbalancer/LoadBalancerPropertiesFactory.java

+		return globalProperties;
+	}
+
+	private LoadBalancerProperties getLoadBalancerServiceProperties(String serviceName) {


Let's rename to getLoadBalancerClientProperties

OlgaMaciaszek · 2021-05-06T14:46:58Z

...ework/cloud/client/loadbalancer/reactive/ReactorLoadBalancerExchangeFilterFunctionTests.java

 		}

+		@Bean
+		public LoadBalancerClientProperties loadBalancerServiceProperties() {


Lets's rename to loadBalancerClientProperties(),

...c/main/java/org/springframework/cloud/client/loadbalancer/LoadBalancerAutoConfiguration.java

OlgaMaciaszek · 2021-05-06T14:51:20Z

...ava/org/springframework/cloud/loadbalancer/core/CachingServiceInstanceListSupplierTests.java

 		}

+		@Bean
+		public LoadBalancerClientProperties loadBalancerServiceProperties() {


Let's rename to loadBalancerClientProperties()

OlgaMaciaszek · 2021-05-06T14:51:41Z

...ava/org/springframework/cloud/loadbalancer/core/CachingServiceInstanceListSupplierTests.java

+
+		@Bean
+		public LoadBalancerPropertiesFactory loadBalancerPropertiesFactory(LoadBalancerProperties globalProperties,
+				LoadBalancerClientProperties serviceProperties) {


Let's rename serviceProperties to clientProperties.

OlgaMaciaszek · 2021-05-06T14:53:04Z

...c/main/java/org/springframework/cloud/client/loadbalancer/LoadBalancerAutoConfiguration.java

+	@Bean
+	@ConditionalOnMissingBean
+	public LoadBalancerPropertiesFactory loadBalancerPropertiesFactory(LoadBalancerProperties globalProperties,
+			LoadBalancerClientProperties servicesProperties,


Let's rename serviceProperties to clientProperties.

OlgaMaciaszek · 2021-05-06T15:37:32Z

Regarding #947 (comment), thanks a lot - that would be great :). Let's finish working on this one first, and then we could switch to the other ones.

OlgaMaciaszek · 2021-05-12T15:07:37Z

@Ferioney - thanks a lot for the work that you've put with this PR. We've had a team discussion on this and we need some time to consider design decisions and also investigate some alternative approaches. We will be adding updates in the actual issue and get back to you once that's done.

spencergibb · 2021-10-22T03:17:35Z

@Ferioney thanks again for the work. It led us in the right direction. @olegz from spring cloud stream gave us the missing piece.

Closing in favor of #1028

Support configuration per client for BlockingLoadBalancedRetryFactory

5fb77bd

spring-cloud-issues added the waiting-for-triage label Apr 22, 2021

Ferioney mentioned this pull request Apr 22, 2021

(PoC) Support configuration per client for BlockingLoadBalancedRetryFactory #942

Closed

spencergibb added in progress and removed waiting-for-triage labels Apr 22, 2021

spencergibb requested review from OlgaMaciaszek and spencergibb April 22, 2021 14:26

OlgaMaciaszek self-assigned this Apr 22, 2021

spencergibb changed the title ~~Issue 914 properties solution~~ Support properties configuration per load balancer client Apr 22, 2021

Ferioney commented Apr 23, 2021

View reviewed changes

OlgaMaciaszek requested changes Apr 26, 2021

View reviewed changes

Andrii Bohutskyi added 2 commits April 27, 2021 10:37

Support configuration per client

60e8626

Support disable retry for reactive lb

f0781ab

Ferioney requested a review from OlgaMaciaszek April 27, 2021 08:22

OlgaMaciaszek requested changes Apr 27, 2021

View reviewed changes

OlgaMaciaszek reviewed Apr 27, 2021

View reviewed changes

...c/main/java/org/springframework/cloud/client/loadbalancer/LoadBalancerAutoConfiguration.java Show resolved Hide resolved

spencergibb requested changes Apr 27, 2021

View reviewed changes

Support disable retry for reactive lb

c509214

Ferioney requested review from OlgaMaciaszek and spencergibb April 30, 2021 14:02

OlgaMaciaszek requested changes May 6, 2021

View reviewed changes

OlgaMaciaszek added icebox and removed in progress labels Jun 10, 2021

spencergibb closed this Oct 22, 2021


		private final LoadBalancerProperties globalProperties;

		private final LoadBalancerClientProperties servicesProperties;


		private final LoadBalancerClientProperties servicesProperties;

		private final boolean isServiceProperties;

Support properties configuration per load balancer client #947

Support properties configuration per load balancer client #947

Uh oh!

Conversation

Ferioney commented Apr 22, 2021 • edited by spencergibb Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

OlgaMaciaszek May 6, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

OlgaMaciaszek left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

OlgaMaciaszek left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

spencergibb left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Ferioney commented Apr 30, 2021

Uh oh!

OlgaMaciaszek commented May 6, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

OlgaMaciaszek left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Ferioney commented Apr 22, 2021 •

edited by spencergibb

Loading

OlgaMaciaszek May 6, 2021 •

edited

Loading

OlgaMaciaszek commented May 6, 2021 •

edited

Loading

OlgaMaciaszek commented May 12, 2021 •

edited

Loading