From 6d7d3c1407a12c2e86f9f67ed05e57b7cc041d2c Mon Sep 17 00:00:00 2001 From: John Blum Date: Tue, 17 Oct 2023 19:30:03 -0700 Subject: [PATCH] Refine JedisConnectionFactory. Introduces private (static) methods (and/or local variables) to simplify and remove duplicate logic while simultaneously removing broken line breaks affecting readability and understanding along with cleaning up some compiler warnings. Simplifies assertions by making consistent use of Spring Frameworks Assert facility rather than unnecessary conditional blocks. Refers to state variables using getter (rather than direct variable reference) where applicable helping to improve extensibility. Deprecates createTopologyProvider(..) in favor of createClusterTopologyProvider(..) for consistent and clarified naming. Closes #2745 --- .../jedis/JedisConnectionFactory.java | 286 ++++++++++++------ 1 file changed, 188 insertions(+), 98 deletions(-) diff --git a/src/main/java/org/springframework/data/redis/connection/jedis/JedisConnectionFactory.java b/src/main/java/org/springframework/data/redis/connection/jedis/JedisConnectionFactory.java index b231900022..9c0385208b 100644 --- a/src/main/java/org/springframework/data/redis/connection/jedis/JedisConnectionFactory.java +++ b/src/main/java/org/springframework/data/redis/connection/jedis/JedisConnectionFactory.java @@ -57,6 +57,7 @@ import org.springframework.data.redis.connection.RedisConfiguration.SentinelConfiguration; import org.springframework.data.redis.connection.RedisConfiguration.WithDatabaseIndex; import org.springframework.data.redis.connection.RedisConfiguration.WithPassword; +import org.springframework.data.redis.connection.jedis.JedisClusterConnection.JedisClusterNodeResourceProvider; import org.springframework.data.redis.connection.jedis.JedisClusterConnection.JedisClusterTopologyProvider; import org.springframework.lang.Nullable; import org.springframework.util.Assert; @@ -92,6 +93,7 @@ * @author Mark Paluch * @author Fu Jian * @author Ajith Kumar + * @author John Blum * @see JedisClientConfiguration * @see Jedis */ @@ -100,8 +102,7 @@ public class JedisConnectionFactory private static final Log log = LogFactory.getLog(JedisConnectionFactory.class); - private static final ExceptionTranslationStrategy EXCEPTION_TRANSLATION = new PassThroughExceptionTranslationStrategy( - JedisExceptionConverter.INSTANCE); + private static final ExceptionTranslationStrategy EXCEPTION_TRANSLATION = jedisExceptionTranslationStrategy(); private boolean convertPipelineAndTxResults = true; @@ -109,9 +110,9 @@ public class JedisConnectionFactory private final AtomicReference state = new AtomicReference<>(State.CREATED); - private @Nullable ClusterCommandExecutor clusterCommandExecutor; + private @Nullable AsyncTaskExecutor asyncExecutor; - private @Nullable AsyncTaskExecutor executor; + private @Nullable ClusterCommandExecutor clusterCommandExecutor; private @Nullable ClusterTopologyProvider topologyProvider; @@ -125,8 +126,7 @@ public class JedisConnectionFactory private @Nullable RedisConfiguration configuration; - private RedisStandaloneConfiguration standaloneConfig = new RedisStandaloneConfiguration("localhost", - Protocol.DEFAULT_PORT); + private RedisStandaloneConfiguration standaloneConfig = defaultStandaloneConfiguration(); /** * Lifecycle state of this factory. @@ -249,8 +249,7 @@ public JedisConnectionFactory(RedisSentinelConfiguration sentinelConfiguration, @Nullable JedisPoolConfig poolConfig) { this.configuration = sentinelConfiguration; - this.clientConfiguration = MutableJedisClientConfiguration - .create(poolConfig != null ? poolConfig : new JedisPoolConfig()); + this.clientConfiguration = MutableJedisClientConfiguration.create(nullSafePoolConfig(poolConfig)); } /** @@ -281,11 +280,17 @@ public JedisConnectionFactory(RedisStandaloneConfiguration standaloneConfigurati this.standaloneConfig = standaloneConfiguration; } + private static RedisStandaloneConfiguration defaultStandaloneConfiguration() { + return new RedisStandaloneConfiguration("localhost", Protocol.DEFAULT_PORT); + } + + private static PassThroughExceptionTranslationStrategy jedisExceptionTranslationStrategy() { + return new PassThroughExceptionTranslationStrategy(JedisExceptionConverter.INSTANCE); + } + ClusterCommandExecutor getRequiredClusterCommandExecutor() { - if (this.clusterCommandExecutor == null) { - throw new IllegalStateException("ClusterCommandExecutor not initialized"); - } + Assert.state(this.clusterCommandExecutor != null, "ClusterCommandExecutor not initialized"); return this.clusterCommandExecutor; } @@ -300,7 +305,7 @@ public void setExecutor(AsyncTaskExecutor executor) { Assert.notNull(executor, "AsyncTaskExecutor must not be null"); - this.executor = executor; + this.asyncExecutor = executor; } /** @@ -356,15 +361,6 @@ public String getPassword() { return getRedisPassword().map(String::new).orElse(null); } - @Nullable - private String getRedisUsername() { - return RedisConfiguration.getUsernameOrElse(this.configuration, standaloneConfig::getUsername); - } - - private RedisPassword getRedisPassword() { - return RedisConfiguration.getPasswordOrElse(this.configuration, standaloneConfig::getPassword); - } - /** * Sets the password used for authenticating with the Redis server. * @@ -452,9 +448,8 @@ public boolean getUsePool() { @Deprecated public void setUsePool(boolean usePool) { - if (isRedisSentinelAware() && !usePool) { - throw new IllegalStateException("Jedis requires pooling for Redis Sentinel use"); - } + Assert.state(!isRedisSentinelAware() || usePool, + "Jedis requires pooling for Redis Sentinel use"); getMutableConfiguration().setUsePooling(usePool); } @@ -466,7 +461,7 @@ public void setUsePool(boolean usePool) { */ @Nullable public GenericObjectPoolConfig getPoolConfig() { - return clientConfiguration.getPoolConfig().orElse(null); + return getClientConfiguration().getPoolConfig().orElse(null); } /** @@ -519,7 +514,7 @@ public void setDatabase(int index) { */ @Nullable public String getClientName() { - return clientConfiguration.getClientName().orElse(null); + return getClientConfiguration().getClientName().orElse(null); } /** @@ -568,7 +563,8 @@ public RedisSentinelConfiguration getSentinelConfiguration() { */ @Nullable public RedisClusterConfiguration getClusterConfiguration() { - return RedisConfiguration.isClusterConfiguration(configuration) ? (RedisClusterConfiguration) configuration : null; + return RedisConfiguration.isClusterConfiguration(configuration) ? (RedisClusterConfiguration) configuration + : null; } /** @@ -628,13 +624,12 @@ private JedisClientConfig createClientConfig(int database, @Nullable String user this.clientConfiguration.getClientName().ifPresent(builder::clientName); builder.connectionTimeoutMillis(getConnectTimeout()); builder.socketTimeoutMillis(getReadTimeout()); - builder.database(database); + password.toOptional().map(String::new).ifPresent(builder::password); if (!ObjectUtils.isEmpty(username)) { builder.user(username); } - password.toOptional().map(String::new).ifPresent(builder::password); if (isUseSsl()) { @@ -649,16 +644,19 @@ private JedisClientConfig createClientConfig(int database, @Nullable String user } JedisClientConfig createSentinelClientConfig(SentinelConfiguration sentinelConfiguration) { - return createClientConfig(0, sentinelConfiguration.getSentinelUsername(), - sentinelConfiguration.getSentinelPassword()); + + String username = sentinelConfiguration.getSentinelUsername(); + RedisPassword password = sentinelConfiguration.getSentinelPassword(); + + return createClientConfig(0, username, password); } @Override public void start() { - State current = this.state.getAndUpdate(state -> isCreatedOrStopped(state) ? State.STARTING : state); + State currentState = this.state.getAndUpdate(state -> isCreatedOrStopped(state) ? State.STARTING : state); - if (isCreatedOrStopped(current)) { + if (isCreatedOrStopped(currentState)) { if (getUsePool() && !isRedisClusterAware()) { this.pool = createPool(); @@ -667,32 +665,29 @@ public void start() { if (isRedisClusterAware()) { this.cluster = createCluster(getClusterConfiguration(), getPoolConfig()); - this.topologyProvider = createTopologyProvider(this.cluster); - this.clusterCommandExecutor = new ClusterCommandExecutor(this.topologyProvider, - new JedisClusterConnection.JedisClusterNodeResourceProvider(this.cluster, this.topologyProvider), - EXCEPTION_TRANSLATION, executor); + this.topologyProvider = createClusterTopologyProvider(this.cluster); + this.clusterCommandExecutor = createClusterCommandExecutor(this.cluster, this.topologyProvider); } this.state.set(State.STARTED); } } - private boolean isCreatedOrStopped(@Nullable State state) { - return State.CREATED.equals(state) || State.STOPPED.equals(state); - } - @Override public void stop() { if (this.state.compareAndSet(State.STARTED, State.STOPPING)) { if (getUsePool() && !isRedisClusterAware()) { - if (this.pool != null) { + + Pool jedisPool = this.pool; + + if (jedisPool != null) { try { - this.pool.close(); + jedisPool.close(); this.pool = null; - } catch (Exception ex) { - log.warn("Cannot properly close Jedis pool", ex); + } catch (Exception cause) { + log.warn("Cannot properly close Jedis pool", cause); } } } @@ -708,12 +703,14 @@ public void stop() { } } - if (this.cluster != null) { + JedisCluster cluster = this.cluster; + + if (cluster != null) { this.topologyProvider = null; try { - this.cluster.close(); + cluster.close(); this.cluster = null; } catch (Exception cause) { log.warn("Cannot properly close Jedis cluster", cause); @@ -741,15 +738,7 @@ public void setPhase(int phase) { @Override public boolean isRunning() { - return State.STARTED.equals(this.state.get()); - } - - private Pool createPool() { - - if (isRedisSentinelAware()) { - return createRedisSentinelPool(getSentinelConfiguration()); - } - return createRedisPool(); + return isStarted(this.state.get()); } /** @@ -761,12 +750,15 @@ private Pool createPool() { */ protected Pool createRedisSentinelPool(RedisSentinelConfiguration config) { - GenericObjectPoolConfig poolConfig = getPoolConfig() != null ? getPoolConfig() : new JedisPoolConfig(); + GenericObjectPoolConfig poolConfig = nullSafePoolConfig(getPoolConfig()); JedisClientConfig sentinelConfig = createSentinelClientConfig(config); - return new JedisSentinelPool(config.getMaster().getName(), convertToJedisSentinelSet(config.getSentinels()), - poolConfig, this.clientConfig, sentinelConfig); + Set sentinelHostsAndPorts = convertToJedisSentinelSet(config.getSentinels()); + + String masterName = config.getMaster().getName(); + + return new JedisSentinelPool(masterName, sentinelHostsAndPorts, poolConfig, this.clientConfig, sentinelConfig); } /** @@ -776,22 +768,58 @@ protected Pool createRedisSentinelPool(RedisSentinelConfiguration config) * @since 1.4 */ protected Pool createRedisPool() { - return new JedisPool(getPoolConfig(), new HostAndPort(getHostName(), getPort()), this.clientConfig); + return new JedisPool(getPoolConfig(), newHostAndPort(getHostName(), getPort()), this.clientConfig); } /** - * Template method to create a {@link ClusterTopologyProvider} given {@link JedisCluster}. Creates - * {@link JedisClusterTopologyProvider} by default. - * - * @param cluster the {@link JedisCluster}, must not be {@literal null}. - * @return the {@link ClusterTopologyProvider}. - * @see JedisClusterTopologyProvider - * @see 2.2 + * @deprecated Use {@link #createClusterTopologyProvider(JedisCluster)} instead. + * @since 2.2 */ + @Deprecated(since = "3.2") protected ClusterTopologyProvider createTopologyProvider(JedisCluster cluster) { return new JedisClusterTopologyProvider(cluster); } + /** + * Template method to create a {@link ClusterTopologyProvider} with the given {@link JedisCluster}. + *

+ * Creates {@link JedisClusterTopologyProvider} by default. + * + * @param cluster reference to the configured {@link JedisCluster} used by the {@link ClusterTopologyProvider} + * to interact with the Redis cluster; must not be {@literal null}. + * @return a new {@link ClusterTopologyProvider}. + * @see org.springframework.data.redis.connection.jedis.JedisClusterConnection.JedisClusterTopologyProvider + * @see redis.clients.jedis.JedisCluster + * @since 3.2 + */ + protected ClusterTopologyProvider createClusterTopologyProvider(JedisCluster cluster) { + return createTopologyProvider(cluster); + } + + /** + * Create a new {@link ClusterCommandExecutor} with the given {@link JedisCluster} and {@link ClusterTopologyProvider}. + *

+ * Creates a {@link JedisClusterNodeResourceProvider} by default. + * + * @param cluster reference to the configured {@link JedisCluster} and {@link ClusterTopologyProvider} used to + * execute Redis commands across the cluster; must not be {@literal null}. + * @param clusterTopologyProvider {@link ClusterTopologyProvider} used to gather information about the current + * Redis cluster topology. + * @return a new {@link ClusterCommandExecutor}. + * @see org.springframework.data.redis.connection.ClusterTopologyProvider + * @see redis.clients.jedis.JedisCluster + * @since 3.2 + */ + protected ClusterCommandExecutor createClusterCommandExecutor(JedisCluster cluster, + ClusterTopologyProvider clusterTopologyProvider) { + + JedisClusterNodeResourceProvider clusterNodeResourceProvider = + new JedisClusterNodeResourceProvider(cluster, clusterTopologyProvider); + + return new ClusterCommandExecutor(clusterTopologyProvider, clusterNodeResourceProvider, EXCEPTION_TRANSLATION, + this.asyncExecutor); + } + /** * Creates {@link JedisCluster} for given {@link RedisClusterConfiguration} and {@link GenericObjectPoolConfig}. * @@ -841,8 +869,9 @@ public RedisConnection getConnection() { sentinelConfig = createSentinelClientConfig(sentinelConfiguration); } - JedisConnection connection = getUsePool() ? new JedisConnection(jedis, this.pool, this.clientConfig, sentinelConfig) - : new JedisConnection(jedis, null, this.clientConfig, sentinelConfig); + Pool pool = getUsePool() ? this.pool : null; + + JedisConnection connection = new JedisConnection(jedis, pool, this.clientConfig, sentinelConfig); connection.setConvertPipelineAndTxResults(convertPipelineAndTxResults); @@ -863,21 +892,18 @@ protected Jedis fetchJedisConnector() { return this.pool.getResource(); } - Jedis jedis = createJedis(); + Jedis jedis = createJedis(getHostName(), getPort(), this.clientConfig); // force initialization (see Jedis issue #82) jedis.connect(); return jedis; + } catch (Exception cause) { throw new RedisConnectionFailureException("Cannot get Jedis connection", cause); } } - private Jedis createJedis() { - return new Jedis(new HostAndPort(getHostName(), getPort()), this.clientConfig); - } - /** * Post process a newly retrieved connection. Useful for decorating or executing initialization commands on a new * connection. This implementation simply returns the connection. @@ -935,10 +961,13 @@ public RedisSentinelConnection getSentinelConnection() { private Jedis getActiveSentinel() { - Assert.isTrue(RedisConfiguration.isSentinelConfiguration(configuration), "SentinelConfig must not be null"); - SentinelConfiguration sentinelConfiguration = (SentinelConfiguration) configuration; + Assert.isTrue(RedisConfiguration.isSentinelConfiguration(this.configuration), + "SentinelConfig must not be null"); + + SentinelConfiguration sentinelConfiguration = (SentinelConfiguration) this.configuration; JedisClientConfig clientConfig = createSentinelClientConfig(sentinelConfiguration); + for (RedisNode node : sentinelConfiguration.getSentinels()) { Jedis jedis = null; @@ -946,13 +975,14 @@ private Jedis getActiveSentinel() { try { - jedis = new Jedis(new HostAndPort(node.getHost(), node.getPort()), clientConfig); + jedis = createJedis(node, clientConfig); + if (jedis.ping().equalsIgnoreCase("pong")) { success = true; return jedis; } - } catch (Exception ex) { - log.warn(String.format("Ping failed for sentinel host: %s", node.getHost()), ex); + } catch (Exception cause) { + log.warn("Ping failed for sentinel host: %s".formatted(node.getHost()), cause); } finally { if (!success && jedis != null) { jedis.close(); @@ -963,6 +993,30 @@ private Jedis getActiveSentinel() { throw new InvalidDataAccessResourceUsageException("No Sentinel found"); } + private boolean isCreatedOrStopped(@Nullable State state) { + return State.CREATED.equals(state) || State.STOPPED.equals(state); + } + + private boolean isStarted(@Nullable State state) { + return State.STARTED.equals(state); + } + + private Jedis createJedis(String hostname, int port, JedisClientConfig config) { + return createJedis(newHostAndPort(hostname, port), config); + } + + private Jedis createJedis(RedisNode node, JedisClientConfig config) { + return createJedis(newHostAndPort(node), config); + } + + private Jedis createJedis(HostAndPort hostPort, JedisClientConfig config) { + return new Jedis(hostPort, config); + } + + private Pool createPool() { + return isRedisSentinelAware() ? createRedisSentinelPool(getSentinelConfiguration()) : createRedisPool(); + } + private static Set convertToJedisSentinelSet(Collection nodes) { if (CollectionUtils.isEmpty(nodes)) { @@ -970,69 +1024,105 @@ private static Set convertToJedisSentinelSet(Collection } Set convertedNodes = new LinkedHashSet<>(nodes.size()); + for (RedisNode node : nodes) { if (node != null) { - convertedNodes.add(new HostAndPort(node.getHost(), node.getPort())); + convertedNodes.add(newHostAndPort(node)); } } + return convertedNodes; } - private int getReadTimeout() { - return Math.toIntExact(clientConfiguration.getReadTimeout().toMillis()); + private static HostAndPort newHostAndPort(String hostname, int port) { + return new HostAndPort(hostname, port); + } + + private static HostAndPort newHostAndPort(RedisNode node) { + return new HostAndPort(node.getHost(), node.getPort()); + } + + private GenericObjectPoolConfig nullSafePoolConfig(@Nullable GenericObjectPoolConfig poolConfig) { + return poolConfig != null ? poolConfig : new JedisPoolConfig(); } private int getConnectTimeout() { return Math.toIntExact(clientConfiguration.getConnectTimeout().toMillis()); } + private int getReadTimeout() { + return Math.toIntExact(clientConfiguration.getReadTimeout().toMillis()); + } + + @Nullable + private String getRedisUsername() { + return RedisConfiguration.getUsernameOrElse(this.configuration, standaloneConfig::getUsername); + } + + private RedisPassword getRedisPassword() { + return RedisConfiguration.getPasswordOrElse(this.configuration, standaloneConfig::getPassword); + } + private MutableJedisClientConfiguration getMutableConfiguration() { Assert.state(clientConfiguration instanceof MutableJedisClientConfiguration, - () -> String.format("Client configuration must be instance of MutableJedisClientConfiguration but is %s", - ClassUtils.getShortName(clientConfiguration.getClass()))); + () -> "Client configuration must be instance of MutableJedisClientConfiguration but is %s" + .formatted(ClassUtils.getShortName(clientConfiguration.getClass()))); return (MutableJedisClientConfiguration) clientConfiguration; } private void assertInitialized() { - State current = state.get(); + State state = this.state.get(); - if (State.STARTED.equals(current)) { + if (isStarted(state)) { return; } - switch (current) { - case CREATED, STOPPED -> throw new IllegalStateException( - String.format("JedisConnectionFactory has been %s. Use start() to initialize it", current)); - case DESTROYED -> throw new IllegalStateException( - "JedisConnectionFactory was destroyed and cannot be used anymore"); - default -> throw new IllegalStateException(String.format("JedisConnectionFactory is %s", current)); + switch (state) { + case CREATED, STOPPED -> + throwIllegalStateException("JedisConnectionFactory has been %s. Call start() to initialize", state); + case DESTROYED -> + throwIllegalStateException("JedisConnectionFactory was destroyed and cannot be used anymore"); + default -> throwIllegalStateException("JedisConnectionFactory is %s", state); } } + private void throwIllegalStateException(String message, Object... args) { + throw new IllegalStateException(message.formatted(args)); + } + /** * Mutable implementation of {@link JedisClientConfiguration}. * * @author Mark Paluch */ + @SuppressWarnings("rawtypes") static class MutableJedisClientConfiguration implements JedisClientConfiguration { - private boolean useSsl; - private @Nullable SSLSocketFactory sslSocketFactory; - private @Nullable SSLParameters sslParameters; - private @Nullable HostnameVerifier hostnameVerifier; private boolean usePooling = true; + private boolean useSsl; + + private Duration connectTimeout = Duration.ofMillis(Protocol.DEFAULT_TIMEOUT); + private Duration readTimeout = Duration.ofMillis(Protocol.DEFAULT_TIMEOUT); + private GenericObjectPoolConfig poolConfig = new JedisPoolConfig(); + + private @Nullable HostnameVerifier hostnameVerifier; + + private @Nullable SSLParameters sslParameters; + + private @Nullable SSLSocketFactory sslSocketFactory; + private @Nullable String clientName; - private Duration readTimeout = Duration.ofMillis(Protocol.DEFAULT_TIMEOUT); - private Duration connectTimeout = Duration.ofMillis(Protocol.DEFAULT_TIMEOUT); public static JedisClientConfiguration create(GenericObjectPoolConfig jedisPoolConfig) { MutableJedisClientConfiguration configuration = new MutableJedisClientConfiguration(); + configuration.setPoolConfig(jedisPoolConfig); + return configuration; }