DATAREDIS-315 - Remove timeout/password from RedisClusterConfiguration.

Removed duplicate configuration options for timeout and password from RedisClusterConfiguration and use the ones provided via the RedisConnectionFactory. Original pull request: spring-projects#158.

Author: christophstrobl

src/main/asciidoc/reference/redis-cluster.adoc

@@ -62,9 +62,7 @@ public class AppConfig {
 
 .Configuration Properties
 - `spring.redis.cluster.nodes`: Comma delimited list of host:port pairs.
-- `spring.redis.cluster.timeout`: Timeout (in milliseconds) for cluster operations.
 - `spring.redis.cluster.max-redirects`: Number of allowed cluster redirections.
-- `spring.redis.cluster.password`: Password to access a password-protected Redis Cluster. Must equal across all nodes.
 ====
 
 NOTE: The initial configuration points driver libraries to an initial set of cluster nodes. Changes resulting from live cluster reconfiguration will only be kept in the native driver and not be written back to the configuration. 

src/main/java/org/springframework/data/redis/connection/RedisClusterConfiguration.java

@@ -1,5 +1,5 @@
 /*
- * Copyright 2015 the original author or authors.
+ * Copyright 2015-2016 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.
@@ -43,14 +43,10 @@
 public class RedisClusterConfiguration {
 
 private static final String REDIS_CLUSTER_NODES_CONFIG_PROPERTY = "spring.redis.cluster.nodes";
-private static final String REDIS_CLUSTER_TIMEOUT_CONFIG_PROPERTY = "spring.redis.cluster.timeout";
 private static final String REDIS_CLUSTER_MAX_REDIRECTS_CONFIG_PROPERTY = "spring.redis.cluster.max-redirects";
-private static final String REDIS_CLUSTER_PASSWORD_PROPERTY = "spring.redis.cluster.password";
 
 private Set<RedisNode> clusterNodes;
-private Long clusterTimeout;
 private Integer maxRedirects;
-private String password;
 
 /**
  * Creates new {@link RedisClusterConfiguration}.
@@ -100,17 +96,10 @@ public RedisClusterConfiguration(PropertySource<?> propertySource) {
 appendClusterNodes(commaDelimitedListToSet(propertySource.getProperty(REDIS_CLUSTER_NODES_CONFIG_PROPERTY)
 .toString()));
 }
-if (propertySource.containsProperty(REDIS_CLUSTER_TIMEOUT_CONFIG_PROPERTY)) {
-this.clusterTimeout = NumberUtils.parseNumber(propertySource.getProperty(REDIS_CLUSTER_TIMEOUT_CONFIG_PROPERTY)
-.toString(), Long.class);
-}
 if (propertySource.containsProperty(REDIS_CLUSTER_MAX_REDIRECTS_CONFIG_PROPERTY)) {
 this.maxRedirects = NumberUtils.parseNumber(
 propertySource.getProperty(REDIS_CLUSTER_MAX_REDIRECTS_CONFIG_PROPERTY).toString(), Integer.class);
 }
-if (propertySource.containsProperty(REDIS_CLUSTER_PASSWORD_PROPERTY)) {
-this.password =propertySource.getProperty(REDIS_CLUSTER_PASSWORD_PROPERTY).toString();
-}
 }
 
 /**
@@ -153,25 +142,11 @@ public void addClusterNode(RedisNode node) {
  * @return
  */
 public RedisClusterConfiguration clusterNode(RedisNode node) {
+
 this.clusterNodes.add(node);
 return this;
 }
 
-/**
- * @return
- */
-public Long getClusterTimeout() {
-return clusterTimeout != null && clusterTimeout > Long.MIN_VALUE ? clusterTimeout : null;
-}
-
-/**
- *
- * @param clusterTimeout
- */
-public void setClusterTimeout(long clusterTimeout) {
-this.clusterTimeout = clusterTimeout;
-}
-
 /**
  * @return
  */
@@ -180,29 +155,14 @@ public Integer getMaxRedirects() {
 }
 
 /**
- *
  * @param maxRedirects
  */
 public void setMaxRedirects(int maxRedirects) {
+
 Assert.isTrue(maxRedirects >= 0, "MaxRedirects must be greater or equal to 0");
 this.maxRedirects = maxRedirects;
 }
 
-/**
- * @return
- */
-public String getPassword() {
-return password;
-}
-
-/**
- *
- * @param password can be {@literal null} or empty.
- */
-public void setPassword(String password) {
-this.password = password;
-}
-
 /**
  * @param host
  * @param port
@@ -235,21 +195,17 @@ private RedisNode readHostAndPortFromString(String hostAndPort) {
  * @param password can be {@literal null} or empty.
  * @return
  */
-private static Map<String, Object> asMap(Collection<String> clusterHostAndPorts, long timeout, int redirects, String password) {
+private static Map<String, Object> asMap(Collection<String> clusterHostAndPorts, long timeout, int redirects,
+String password) {
 
 notNull(clusterHostAndPorts, "ClusterHostAndPorts must not be null!");
 
 Map<String, Object> map = new HashMap<String, Object>();
 map.put(REDIS_CLUSTER_NODES_CONFIG_PROPERTY, StringUtils.collectionToCommaDelimitedString(clusterHostAndPorts));
-if (timeout >= 0) {
-map.put(REDIS_CLUSTER_TIMEOUT_CONFIG_PROPERTY, Long.valueOf(timeout));
-}
+
 if (redirects >= 0) {
 map.put(REDIS_CLUSTER_MAX_REDIRECTS_CONFIG_PROPERTY, Integer.valueOf(redirects));
 }
-if (StringUtils.hasText(password)) {
-map.put(REDIS_CLUSTER_PASSWORD_PROPERTY, password);
-}
 
 return map;
 }

src/main/java/org/springframework/data/redis/connection/jedis/JedisConnectionFactory.java

@@ -1,5 +1,5 @@
 /*
- * Copyright 2011-2015 the original author or authors.
+ * Copyright 2011-2016 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.
@@ -293,11 +293,10 @@ protected JedisCluster createCluster(RedisClusterConfiguration clusterConfig, Ge
 hostAndPort.add(new HostAndPort(node.getHost(), node.getPort()));
 }
 
-int timeout = clusterConfig.getClusterTimeout() != null ? clusterConfig.getClusterTimeout().intValue() : this.timeout;
 int redirects = clusterConfig.getMaxRedirects() != null ? clusterConfig.getMaxRedirects().intValue() : 5;
-
-if (StringUtils.hasText(clusterConfig.getPassword())) {
-throw new UnsupportedOperationException("Jedis does not support password protected Redis Cluster configurations");
+
+if (StringUtils.hasText(getPassword())) {
+throw new IllegalArgumentException("Jedis does not support password protected Redis Cluster configurations!");
 }
 
 if (poolConfig != null) {

src/main/java/org/springframework/data/redis/connection/lettuce/LettuceConnectionFactory.java

@@ -1,5 +1,5 @@
 /*
- * Copyright 2011-2015 the original author or authors.
+ * Copyright 2011-2016 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.
@@ -40,6 +40,7 @@
 import org.springframework.data.redis.connection.RedisSentinelConfiguration;
 import org.springframework.data.redis.connection.RedisSentinelConnection;
 import org.springframework.util.Assert;
+import org.springframework.util.StringUtils;
 
 import com.lambdaworks.redis.AbstractRedisClient;
 import com.lambdaworks.redis.LettuceFutures;
@@ -49,7 +50,6 @@
 import com.lambdaworks.redis.RedisFuture;
 import com.lambdaworks.redis.RedisURI;
 import com.lambdaworks.redis.cluster.RedisClusterClient;
-import org.springframework.util.StringUtils;
 
 /**
  * Connection factory creating <a href="http://github.com/mp911de/lettuce">Lettuce</a>-based connections.
@@ -465,12 +465,10 @@ private AbstractRedisClient createRedisClient() {
 
 List<RedisURI> initialUris = new ArrayList<RedisURI>();
 for (RedisNode node : this.clusterConfiguration.getClusterNodes()) {
-long timeout = this.clusterConfiguration.getClusterTimeout() != null ? this.clusterConfiguration.getClusterTimeout() : this.timeout;
+
 RedisURI redisURI = new RedisURI(node.getHost(), node.getPort(), timeout, TimeUnit.MILLISECONDS);
 
-if(StringUtils.hasText(this.clusterConfiguration.getPassword())){
-redisURI.setPassword(this.clusterConfiguration.getPassword());
-}else if(StringUtils.hasText(password)){
+if (StringUtils.hasText(password)) {
 redisURI.setPassword(password);
 }
 

src/test/java/org/springframework/data/redis/connection/RedisClusterConfigurationUnitTests.java

@@ -1,5 +1,5 @@
 /*
- * Copyright 2015 the original author or authors.
+ * Copyright 2015-2016 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.
@@ -50,7 +50,6 @@ public void shouldCreateRedisClusterConfigurationCorrectly() {
 
 assertThat(config.getClusterNodes().size(), is(1));
 assertThat(config.getClusterNodes(), hasItems(new RedisNode("127.0.0.1", 123)));
-assertThat(config.getClusterTimeout(), nullValue());
 assertThat(config.getMaxRedirects(), nullValue());
 }
 
@@ -112,7 +111,6 @@ public void shouldNotFailWhenGivenPropertySourceNotContainingRelevantProperties(
 RedisClusterConfiguration config = new RedisClusterConfiguration(new MockPropertySource());
 
 assertThat(config.getMaxRedirects(), nullValue());
-assertThat(config.getClusterTimeout(), nullValue());
 assertThat(config.getClusterNodes().size(), is(0));
 }
 
@@ -123,17 +121,13 @@ public void shouldNotFailWhenGivenPropertySourceNotContainingRelevantProperties(
 public void shouldBeCreatedCorrecltyGivenValidPropertySourceWithSingleHostPort() {
 
 MockPropertySource propertySource = new MockPropertySource();
-propertySource.setProperty("spring.redis.cluster.timeout", "10");
 propertySource.setProperty("spring.redis.cluster.nodes", HOST_AND_PORT_1);
 propertySource.setProperty("spring.redis.cluster.max-redirects", "5");
-propertySource.setProperty("spring.redis.cluster.password", "foobar");
 
 RedisClusterConfiguration config = new RedisClusterConfiguration(propertySource);
 
 assertThat(config.getMaxRedirects(), is(5));
-assertThat(config.getClusterTimeout(), is(10L));
 assertThat(config.getClusterNodes(), hasItems(new RedisNode("127.0.0.1", 123)));
-assertThat(config.getPassword(), is("foobar"));
 }
 
 /**
@@ -143,15 +137,13 @@ public void shouldBeCreatedCorrecltyGivenValidPropertySourceWithSingleHostPort()
 public void shouldBeCreatedCorrecltyGivenValidPropertySourceWithMultipleHostPort() {
 
 MockPropertySource propertySource = new MockPropertySource();
-propertySource.setProperty("spring.redis.cluster.timeout", "10");
 propertySource.setProperty("spring.redis.cluster.nodes",
 StringUtils.collectionToCommaDelimitedString(Arrays.asList(HOST_AND_PORT_1, HOST_AND_PORT_2, HOST_AND_PORT_3)));
 propertySource.setProperty("spring.redis.cluster.max-redirects", "5");
 
 RedisClusterConfiguration config = new RedisClusterConfiguration(propertySource);
 
 assertThat(config.getMaxRedirects(), is(5));
-assertThat(config.getClusterTimeout(), is(10L));
 assertThat(config.getClusterNodes(),
 hasItems(new RedisNode("127.0.0.1", 123), new RedisNode("localhost", 456), new RedisNode("localhost", 789)));
 }

src/test/java/org/springframework/data/redis/connection/lettuce/LettuceConnectionFactoryUnitTests.java

@@ -1,5 +1,5 @@
 /*
- * Copyright 2015 the original author or authors.
+ * Copyright 2015-2016 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.
@@ -15,32 +15,86 @@
  */
 package org.springframework.data.redis.connection.lettuce;
 
+import static org.hamcrest.core.Is.*;
+import static org.hamcrest.core.IsEqual.*;
 import static org.hamcrest.core.IsInstanceOf.*;
 import static org.junit.Assert.*;
 import static org.springframework.test.util.ReflectionTestUtils.*;
 
+import java.util.concurrent.TimeUnit;
+
+import org.junit.Before;
 import org.junit.Test;
 import org.springframework.data.redis.connection.RedisClusterConfiguration;
 
+import com.lambdaworks.redis.AbstractRedisClient;
+import com.lambdaworks.redis.RedisURI;
 import com.lambdaworks.redis.cluster.RedisClusterClient;
 
 /**
  * @author Christoph Strobl
  */
 public class LettuceConnectionFactoryUnitTests {
 
-private static final RedisClusterConfiguration CLUSTER_CONFIG = new RedisClusterConfiguration().clusterNode(
-"127.0.0.1", 6379).clusterNode("127.0.0.1", 6380);
+RedisClusterConfiguration clusterConfig;
+
+@Before
+public void setUp() {
+clusterConfig = new RedisClusterConfiguration().clusterNode("127.0.0.1", 6379).clusterNode("127.0.0.1", 6380);
+}
 
 /**
  * @see DATAREDIS-315
  */
 @Test
 public void shouldInitClientCorrectlyWhenClusterConfigPresent() {
 
-LettuceConnectionFactory connectionFactory = new LettuceConnectionFactory(CLUSTER_CONFIG);
+LettuceConnectionFactory connectionFactory = new LettuceConnectionFactory(clusterConfig);
 connectionFactory.afterPropertiesSet();
 
 assertThat(getField(connectionFactory, "client"), instanceOf(RedisClusterClient.class));
 }
+
+/**
+ * @see DATAREDIS-315
+ */
+@Test
+@SuppressWarnings("unchecked")
+public void timeoutShouldBeSetCorrectlyOnClusterClient() {
+
+LettuceConnectionFactory connectionFactory = new LettuceConnectionFactory(clusterConfig);
+connectionFactory.setTimeout(1000);
+connectionFactory.afterPropertiesSet();
+
+AbstractRedisClient client = (AbstractRedisClient) getField(connectionFactory, "client");
+assertThat(client, instanceOf(RedisClusterClient.class));
+
+Iterable<RedisURI> initialUris = (Iterable<RedisURI>) getField(client, "initialUris");
+
+for (RedisURI uri : initialUris) {
+assertThat(uri.getTimeout(), is(equalTo(connectionFactory.getTimeout())));
+assertThat(uri.getUnit(), is(equalTo(TimeUnit.MILLISECONDS)));
+}
+}
+
+/**
+ * @see DATAREDIS-315
+ */
+@Test
+@SuppressWarnings("unchecked")
+public void passwordShouldBeSetCorrectlyOnClusterClient() {
+
+LettuceConnectionFactory connectionFactory = new LettuceConnectionFactory(clusterConfig);
+connectionFactory.setPassword("o_O");
+connectionFactory.afterPropertiesSet();
+
+AbstractRedisClient client = (AbstractRedisClient) getField(connectionFactory, "client");
+assertThat(client, instanceOf(RedisClusterClient.class));
+
+Iterable<RedisURI> initialUris = (Iterable<RedisURI>) getField(client, "initialUris");
+
+for (RedisURI uri : initialUris) {
+assertThat(uri.getPassword(), is(equalTo(connectionFactory.getPassword().toCharArray())));
+}
+}
 }