From 2527fcac9ccb903de1fa8a6322bb9406b42e871f Mon Sep 17 00:00:00 2001 From: anshlykov Date: Sat, 29 Aug 2020 02:43:09 +0300 Subject: [PATCH 1/2] Add separate property for Redis read and connection timeout See gh-23137 --- .../redis/JedisConnectionConfiguration.java | 10 +++++--- .../redis/LettuceConnectionConfiguration.java | 17 ++++++++++--- .../data/redis/RedisProperties.java | 15 ++++++++++- .../RedisAutoConfigurationJedisTests.java | 25 ++++++++++++++----- .../redis/RedisAutoConfigurationTests.java | 22 +++++++++++++--- 5 files changed, 73 insertions(+), 16 deletions(-) diff --git a/spring-boot-project/spring-boot-autoconfigure/src/main/java/org/springframework/boot/autoconfigure/data/redis/JedisConnectionConfiguration.java b/spring-boot-project/spring-boot-autoconfigure/src/main/java/org/springframework/boot/autoconfigure/data/redis/JedisConnectionConfiguration.java index 91331a5e9d..af47306b90 100644 --- a/spring-boot-project/spring-boot-autoconfigure/src/main/java/org/springframework/boot/autoconfigure/data/redis/JedisConnectionConfiguration.java +++ b/spring-boot-project/spring-boot-autoconfigure/src/main/java/org/springframework/boot/autoconfigure/data/redis/JedisConnectionConfiguration.java @@ -92,9 +92,13 @@ class JedisConnectionConfiguration extends RedisConnectionConfiguration { if (getProperties().isSsl()) { builder.useSsl(); } - if (getProperties().getTimeout() != null) { - Duration timeout = getProperties().getTimeout(); - builder.readTimeout(timeout).connectTimeout(timeout); + Duration connectionTimeout = getProperties().getConnectionTimeout(); + if (connectionTimeout != null) { + builder.connectTimeout(connectionTimeout); + } + Duration timeout = getProperties().getTimeout(); + if (timeout != null) { + builder.readTimeout(timeout); } if (StringUtils.hasText(getProperties().getClientName())) { builder.clientName(getProperties().getClientName()); diff --git a/spring-boot-project/spring-boot-autoconfigure/src/main/java/org/springframework/boot/autoconfigure/data/redis/LettuceConnectionConfiguration.java b/spring-boot-project/spring-boot-autoconfigure/src/main/java/org/springframework/boot/autoconfigure/data/redis/LettuceConnectionConfiguration.java index f49acdbfe9..b2aafb3027 100644 --- a/spring-boot-project/spring-boot-autoconfigure/src/main/java/org/springframework/boot/autoconfigure/data/redis/LettuceConnectionConfiguration.java +++ b/spring-boot-project/spring-boot-autoconfigure/src/main/java/org/springframework/boot/autoconfigure/data/redis/LettuceConnectionConfiguration.java @@ -17,9 +17,11 @@ package org.springframework.boot.autoconfigure.data.redis; import java.net.UnknownHostException; +import java.time.Duration; import io.lettuce.core.ClientOptions; import io.lettuce.core.RedisClient; +import io.lettuce.core.SocketOptions; import io.lettuce.core.TimeoutOptions; import io.lettuce.core.cluster.ClusterClientOptions; import io.lettuce.core.cluster.ClusterTopologyRefreshOptions; @@ -96,7 +98,15 @@ class LettuceConnectionConfiguration extends RedisConnectionConfiguration { if (StringUtils.hasText(getProperties().getUrl())) { customizeConfigurationFromUrl(builder); } - builder.clientOptions(initializeClientOptionsBuilder().timeoutOptions(TimeoutOptions.enabled()).build()); + + ClientOptions.Builder clientOptionsBuilder = initializeClientOptionsBuilder(); + Duration connectionTimeout = getProperties().getConnectionTimeout(); + if (connectionTimeout != null) { + SocketOptions socketOptions = SocketOptions.builder().connectTimeout(connectionTimeout).build(); + clientOptionsBuilder.socketOptions(socketOptions); + } + builder.clientOptions(clientOptionsBuilder.timeoutOptions(TimeoutOptions.enabled()).build()); + builder.clientResources(clientResources); builderCustomizers.orderedStream().forEach((customizer) -> customizer.customize(builder)); return builder.build(); @@ -114,8 +124,9 @@ class LettuceConnectionConfiguration extends RedisConnectionConfiguration { if (getProperties().isSsl()) { builder.useSsl(); } - if (getProperties().getTimeout() != null) { - builder.commandTimeout(getProperties().getTimeout()); + Duration timeout = getProperties().getTimeout(); + if (timeout != null) { + builder.commandTimeout(timeout); } if (getProperties().getLettuce() != null) { RedisProperties.Lettuce lettuce = getProperties().getLettuce(); diff --git a/spring-boot-project/spring-boot-autoconfigure/src/main/java/org/springframework/boot/autoconfigure/data/redis/RedisProperties.java b/spring-boot-project/spring-boot-autoconfigure/src/main/java/org/springframework/boot/autoconfigure/data/redis/RedisProperties.java index d8c12b6148..0e665f19b2 100644 --- a/spring-boot-project/spring-boot-autoconfigure/src/main/java/org/springframework/boot/autoconfigure/data/redis/RedisProperties.java +++ b/spring-boot-project/spring-boot-autoconfigure/src/main/java/org/springframework/boot/autoconfigure/data/redis/RedisProperties.java @@ -67,10 +67,15 @@ public class RedisProperties { private boolean ssl; /** - * Connection timeout. + * Read timeout. */ private Duration timeout; + /** + * Connection timeout. + */ + private Duration connectionTimeout; + /** * Client name to be set on connections with CLIENT SETNAME. */ @@ -145,6 +150,14 @@ public class RedisProperties { return this.timeout; } + public Duration getConnectionTimeout() { + return this.connectionTimeout; + } + + public void setConnectionTimeout(Duration connectionTimeout) { + this.connectionTimeout = connectionTimeout; + } + public String getClientName() { return this.clientName; } diff --git a/spring-boot-project/spring-boot-autoconfigure/src/test/java/org/springframework/boot/autoconfigure/data/redis/RedisAutoConfigurationJedisTests.java b/spring-boot-project/spring-boot-autoconfigure/src/test/java/org/springframework/boot/autoconfigure/data/redis/RedisAutoConfigurationJedisTests.java index 3e23b540cf..f4e3cf6085 100644 --- a/spring-boot-project/spring-boot-autoconfigure/src/test/java/org/springframework/boot/autoconfigure/data/redis/RedisAutoConfigurationJedisTests.java +++ b/spring-boot-project/spring-boot-autoconfigure/src/test/java/org/springframework/boot/autoconfigure/data/redis/RedisAutoConfigurationJedisTests.java @@ -16,13 +16,14 @@ package org.springframework.boot.autoconfigure.data.redis; +import io.lettuce.core.RedisClient; import org.junit.jupiter.api.Test; import org.springframework.beans.BeansException; import org.springframework.beans.factory.config.BeanPostProcessor; import org.springframework.boot.autoconfigure.AutoConfigurations; +import org.springframework.boot.test.context.FilteredClassLoader; import org.springframework.boot.test.context.runner.ApplicationContextRunner; -import org.springframework.boot.testsupport.classpath.ClassPathExclusions; import org.springframework.context.annotation.Bean; import org.springframework.context.annotation.Configuration; import org.springframework.data.redis.connection.RedisConnectionFactory; @@ -37,11 +38,11 @@ import static org.assertj.core.api.Assertions.assertThat; * @author Mark Paluch * @author Stephane Nicoll */ -@ClassPathExclusions("lettuce-core-*.jar") class RedisAutoConfigurationJedisTests { private final ApplicationContextRunner contextRunner = new ApplicationContextRunner() - .withConfiguration(AutoConfigurations.of(RedisAutoConfiguration.class)); + .withConfiguration(AutoConfigurations.of(RedisAutoConfiguration.class)) + .withClassLoader(new FilteredClassLoader(RedisClient.class)); @Test void connectionFactoryDefaultsToJedis() { @@ -136,11 +137,23 @@ class RedisAutoConfigurationJedisTests { } @Test - void testRedisConfigurationWithTimeout() { - this.contextRunner.withPropertyValues("spring.redis.host:foo", "spring.redis.timeout:100").run((context) -> { + void testRedisConfigurationWithTimeoutAndConnectionTimeout() { + this.contextRunner.withPropertyValues("spring.redis.host:foo", "spring.redis.timeout:250", + "spring.redis.connection-timeout:1000").run((context) -> { + JedisConnectionFactory cf = context.getBean(JedisConnectionFactory.class); + assertThat(cf.getHostName()).isEqualTo("foo"); + assertThat(cf.getTimeout()).isEqualTo(250); + assertThat(cf.getClientConfiguration().getConnectTimeout().toMillis()).isEqualTo(1000); + }); + } + + @Test + void testRedisConfigurationWithDefaultTimeouts() { + this.contextRunner.withPropertyValues("spring.redis.host:foo").run((context) -> { JedisConnectionFactory cf = context.getBean(JedisConnectionFactory.class); assertThat(cf.getHostName()).isEqualTo("foo"); - assertThat(cf.getTimeout()).isEqualTo(100); + assertThat(cf.getTimeout()).isEqualTo(2000); + assertThat(cf.getClientConfiguration().getConnectTimeout().toMillis()).isEqualTo(2000); }); } diff --git a/spring-boot-project/spring-boot-autoconfigure/src/test/java/org/springframework/boot/autoconfigure/data/redis/RedisAutoConfigurationTests.java b/spring-boot-project/spring-boot-autoconfigure/src/test/java/org/springframework/boot/autoconfigure/data/redis/RedisAutoConfigurationTests.java index 7e9a474ab0..835ad98755 100644 --- a/spring-boot-project/spring-boot-autoconfigure/src/test/java/org/springframework/boot/autoconfigure/data/redis/RedisAutoConfigurationTests.java +++ b/spring-boot-project/spring-boot-autoconfigure/src/test/java/org/springframework/boot/autoconfigure/data/redis/RedisAutoConfigurationTests.java @@ -168,11 +168,27 @@ class RedisAutoConfigurationTests { } @Test - void testRedisConfigurationWithTimeout() { - this.contextRunner.withPropertyValues("spring.redis.host:foo", "spring.redis.timeout:100").run((context) -> { + void testRedisConfigurationWithTimeoutAndConnectionTimeout() { + this.contextRunner.withPropertyValues("spring.redis.host:foo", "spring.redis.timeout:250", + "spring.redis.connection-timeout:1000").run((context) -> { + LettuceConnectionFactory cf = context.getBean(LettuceConnectionFactory.class); + assertThat(cf.getHostName()).isEqualTo("foo"); + assertThat(cf.getTimeout()).isEqualTo(250); + long actualConnectionTimeout = cf.getClientConfiguration().getClientOptions().get() + .getSocketOptions().getConnectTimeout().toMillis(); + assertThat(actualConnectionTimeout).isEqualTo(1000); + }); + } + + @Test + void testRedisConfigurationWithDefaultTimeouts() { + this.contextRunner.withPropertyValues("spring.redis.host:foo").run((context) -> { LettuceConnectionFactory cf = context.getBean(LettuceConnectionFactory.class); assertThat(cf.getHostName()).isEqualTo("foo"); - assertThat(cf.getTimeout()).isEqualTo(100); + assertThat(cf.getTimeout()).isEqualTo(60000); + long actualConnectionTimeout = cf.getClientConfiguration().getClientOptions().get().getSocketOptions() + .getConnectTimeout().toMillis(); + assertThat(actualConnectionTimeout).isEqualTo(10000); }); } From 7a8b7b9fa7fb95f3be779b271c180f825dc8d39c Mon Sep 17 00:00:00 2001 From: Stephane Nicoll Date: Fri, 11 Sep 2020 11:34:41 +0200 Subject: [PATCH 2/2] Polish "Add separate property for Redis read and connection timeout" See gh-23137 --- .../redis/JedisConnectionConfiguration.java | 21 +++++----------- .../redis/LettuceConnectionConfiguration.java | 24 +++++++++---------- .../data/redis/RedisProperties.java | 10 ++++---- .../RedisAutoConfigurationJedisTests.java | 11 ++++----- .../redis/RedisAutoConfigurationTests.java | 14 +++++------ 5 files changed, 34 insertions(+), 46 deletions(-) diff --git a/spring-boot-project/spring-boot-autoconfigure/src/main/java/org/springframework/boot/autoconfigure/data/redis/JedisConnectionConfiguration.java b/spring-boot-project/spring-boot-autoconfigure/src/main/java/org/springframework/boot/autoconfigure/data/redis/JedisConnectionConfiguration.java index af47306b90..19eb353a86 100644 --- a/spring-boot-project/spring-boot-autoconfigure/src/main/java/org/springframework/boot/autoconfigure/data/redis/JedisConnectionConfiguration.java +++ b/spring-boot-project/spring-boot-autoconfigure/src/main/java/org/springframework/boot/autoconfigure/data/redis/JedisConnectionConfiguration.java @@ -17,7 +17,6 @@ package org.springframework.boot.autoconfigure.data.redis; import java.net.UnknownHostException; -import java.time.Duration; import org.apache.commons.pool2.impl.GenericObjectPool; import redis.clients.jedis.Jedis; @@ -27,6 +26,7 @@ import org.springframework.beans.factory.ObjectProvider; import org.springframework.boot.autoconfigure.condition.ConditionalOnClass; import org.springframework.boot.autoconfigure.condition.ConditionalOnMissingBean; import org.springframework.boot.autoconfigure.condition.ConditionalOnProperty; +import org.springframework.boot.context.properties.PropertyMapper; import org.springframework.context.annotation.Bean; import org.springframework.context.annotation.Configuration; import org.springframework.data.redis.connection.RedisClusterConfiguration; @@ -89,20 +89,11 @@ class JedisConnectionConfiguration extends RedisConnectionConfiguration { } private JedisClientConfigurationBuilder applyProperties(JedisClientConfigurationBuilder builder) { - if (getProperties().isSsl()) { - builder.useSsl(); - } - Duration connectionTimeout = getProperties().getConnectionTimeout(); - if (connectionTimeout != null) { - builder.connectTimeout(connectionTimeout); - } - Duration timeout = getProperties().getTimeout(); - if (timeout != null) { - builder.readTimeout(timeout); - } - if (StringUtils.hasText(getProperties().getClientName())) { - builder.clientName(getProperties().getClientName()); - } + PropertyMapper map = PropertyMapper.get().alwaysApplyingWhenNonNull(); + map.from(getProperties().isSsl()).whenTrue().toCall(builder::useSsl); + map.from(getProperties().getTimeout()).to(builder::readTimeout); + map.from(getProperties().getConnectTimeout()).to(builder::connectTimeout); + map.from(getProperties().getClientName()).whenHasText().to(builder::clientName); return builder; } diff --git a/spring-boot-project/spring-boot-autoconfigure/src/main/java/org/springframework/boot/autoconfigure/data/redis/LettuceConnectionConfiguration.java b/spring-boot-project/spring-boot-autoconfigure/src/main/java/org/springframework/boot/autoconfigure/data/redis/LettuceConnectionConfiguration.java index b2aafb3027..a813084f44 100644 --- a/spring-boot-project/spring-boot-autoconfigure/src/main/java/org/springframework/boot/autoconfigure/data/redis/LettuceConnectionConfiguration.java +++ b/spring-boot-project/spring-boot-autoconfigure/src/main/java/org/springframework/boot/autoconfigure/data/redis/LettuceConnectionConfiguration.java @@ -98,15 +98,7 @@ class LettuceConnectionConfiguration extends RedisConnectionConfiguration { if (StringUtils.hasText(getProperties().getUrl())) { customizeConfigurationFromUrl(builder); } - - ClientOptions.Builder clientOptionsBuilder = initializeClientOptionsBuilder(); - Duration connectionTimeout = getProperties().getConnectionTimeout(); - if (connectionTimeout != null) { - SocketOptions socketOptions = SocketOptions.builder().connectTimeout(connectionTimeout).build(); - clientOptionsBuilder.socketOptions(socketOptions); - } - builder.clientOptions(clientOptionsBuilder.timeoutOptions(TimeoutOptions.enabled()).build()); - + builder.clientOptions(createClientOptions()); builder.clientResources(clientResources); builderCustomizers.orderedStream().forEach((customizer) -> customizer.customize(builder)); return builder.build(); @@ -124,9 +116,8 @@ class LettuceConnectionConfiguration extends RedisConnectionConfiguration { if (getProperties().isSsl()) { builder.useSsl(); } - Duration timeout = getProperties().getTimeout(); - if (timeout != null) { - builder.commandTimeout(timeout); + if (getProperties().getTimeout() != null) { + builder.commandTimeout(getProperties().getTimeout()); } if (getProperties().getLettuce() != null) { RedisProperties.Lettuce lettuce = getProperties().getLettuce(); @@ -140,6 +131,15 @@ class LettuceConnectionConfiguration extends RedisConnectionConfiguration { return builder; } + private ClientOptions createClientOptions() { + ClientOptions.Builder builder = initializeClientOptionsBuilder(); + Duration connectTimeout = getProperties().getConnectTimeout(); + if (connectTimeout != null) { + builder.socketOptions(SocketOptions.builder().connectTimeout(connectTimeout).build()); + } + return builder.timeoutOptions(TimeoutOptions.enabled()).build(); + } + private ClientOptions.Builder initializeClientOptionsBuilder() { if (getProperties().getCluster() != null) { ClusterClientOptions.Builder builder = ClusterClientOptions.builder(); diff --git a/spring-boot-project/spring-boot-autoconfigure/src/main/java/org/springframework/boot/autoconfigure/data/redis/RedisProperties.java b/spring-boot-project/spring-boot-autoconfigure/src/main/java/org/springframework/boot/autoconfigure/data/redis/RedisProperties.java index 0e665f19b2..a9607fda4c 100644 --- a/spring-boot-project/spring-boot-autoconfigure/src/main/java/org/springframework/boot/autoconfigure/data/redis/RedisProperties.java +++ b/spring-boot-project/spring-boot-autoconfigure/src/main/java/org/springframework/boot/autoconfigure/data/redis/RedisProperties.java @@ -74,7 +74,7 @@ public class RedisProperties { /** * Connection timeout. */ - private Duration connectionTimeout; + private Duration connectTimeout; /** * Client name to be set on connections with CLIENT SETNAME. @@ -150,12 +150,12 @@ public class RedisProperties { return this.timeout; } - public Duration getConnectionTimeout() { - return this.connectionTimeout; + public Duration getConnectTimeout() { + return this.connectTimeout; } - public void setConnectionTimeout(Duration connectionTimeout) { - this.connectionTimeout = connectionTimeout; + public void setConnectTimeout(Duration connectTimeout) { + this.connectTimeout = connectTimeout; } public String getClientName() { diff --git a/spring-boot-project/spring-boot-autoconfigure/src/test/java/org/springframework/boot/autoconfigure/data/redis/RedisAutoConfigurationJedisTests.java b/spring-boot-project/spring-boot-autoconfigure/src/test/java/org/springframework/boot/autoconfigure/data/redis/RedisAutoConfigurationJedisTests.java index f4e3cf6085..4d44b2a638 100644 --- a/spring-boot-project/spring-boot-autoconfigure/src/test/java/org/springframework/boot/autoconfigure/data/redis/RedisAutoConfigurationJedisTests.java +++ b/spring-boot-project/spring-boot-autoconfigure/src/test/java/org/springframework/boot/autoconfigure/data/redis/RedisAutoConfigurationJedisTests.java @@ -16,14 +16,13 @@ package org.springframework.boot.autoconfigure.data.redis; -import io.lettuce.core.RedisClient; import org.junit.jupiter.api.Test; import org.springframework.beans.BeansException; import org.springframework.beans.factory.config.BeanPostProcessor; import org.springframework.boot.autoconfigure.AutoConfigurations; -import org.springframework.boot.test.context.FilteredClassLoader; import org.springframework.boot.test.context.runner.ApplicationContextRunner; +import org.springframework.boot.testsupport.classpath.ClassPathExclusions; import org.springframework.context.annotation.Bean; import org.springframework.context.annotation.Configuration; import org.springframework.data.redis.connection.RedisConnectionFactory; @@ -38,11 +37,11 @@ import static org.assertj.core.api.Assertions.assertThat; * @author Mark Paluch * @author Stephane Nicoll */ +@ClassPathExclusions("lettuce-core-*.jar") class RedisAutoConfigurationJedisTests { private final ApplicationContextRunner contextRunner = new ApplicationContextRunner() - .withConfiguration(AutoConfigurations.of(RedisAutoConfiguration.class)) - .withClassLoader(new FilteredClassLoader(RedisClient.class)); + .withConfiguration(AutoConfigurations.of(RedisAutoConfiguration.class)); @Test void connectionFactoryDefaultsToJedis() { @@ -137,9 +136,9 @@ class RedisAutoConfigurationJedisTests { } @Test - void testRedisConfigurationWithTimeoutAndConnectionTimeout() { + void testRedisConfigurationWithTimeoutAndConnectTimeout() { this.contextRunner.withPropertyValues("spring.redis.host:foo", "spring.redis.timeout:250", - "spring.redis.connection-timeout:1000").run((context) -> { + "spring.redis.connect-timeout:1000").run((context) -> { JedisConnectionFactory cf = context.getBean(JedisConnectionFactory.class); assertThat(cf.getHostName()).isEqualTo("foo"); assertThat(cf.getTimeout()).isEqualTo(250); diff --git a/spring-boot-project/spring-boot-autoconfigure/src/test/java/org/springframework/boot/autoconfigure/data/redis/RedisAutoConfigurationTests.java b/spring-boot-project/spring-boot-autoconfigure/src/test/java/org/springframework/boot/autoconfigure/data/redis/RedisAutoConfigurationTests.java index 835ad98755..b0095a94b1 100644 --- a/spring-boot-project/spring-boot-autoconfigure/src/test/java/org/springframework/boot/autoconfigure/data/redis/RedisAutoConfigurationTests.java +++ b/spring-boot-project/spring-boot-autoconfigure/src/test/java/org/springframework/boot/autoconfigure/data/redis/RedisAutoConfigurationTests.java @@ -168,15 +168,14 @@ class RedisAutoConfigurationTests { } @Test - void testRedisConfigurationWithTimeoutAndConnectionTimeout() { + void testRedisConfigurationWithTimeoutAndConnectTimeout() { this.contextRunner.withPropertyValues("spring.redis.host:foo", "spring.redis.timeout:250", - "spring.redis.connection-timeout:1000").run((context) -> { + "spring.redis.connect-timeout:1000").run((context) -> { LettuceConnectionFactory cf = context.getBean(LettuceConnectionFactory.class); assertThat(cf.getHostName()).isEqualTo("foo"); assertThat(cf.getTimeout()).isEqualTo(250); - long actualConnectionTimeout = cf.getClientConfiguration().getClientOptions().get() - .getSocketOptions().getConnectTimeout().toMillis(); - assertThat(actualConnectionTimeout).isEqualTo(1000); + assertThat(cf.getClientConfiguration().getClientOptions().get().getSocketOptions() + .getConnectTimeout().toMillis()).isEqualTo(1000); }); } @@ -186,9 +185,8 @@ class RedisAutoConfigurationTests { LettuceConnectionFactory cf = context.getBean(LettuceConnectionFactory.class); assertThat(cf.getHostName()).isEqualTo("foo"); assertThat(cf.getTimeout()).isEqualTo(60000); - long actualConnectionTimeout = cf.getClientConfiguration().getClientOptions().get().getSocketOptions() - .getConnectTimeout().toMillis(); - assertThat(actualConnectionTimeout).isEqualTo(10000); + assertThat(cf.getClientConfiguration().getClientOptions().get().getSocketOptions().getConnectTimeout() + .toMillis()).isEqualTo(10000); }); }