Refine `JedisConnectionFactory` #2746

jxblum · 2023-10-18T02:39:45Z

mp911de

Did a first review pass. The majority of changes impose a different style, resulting in code that requires more effort to read (harder to read) and that isn't an improvement.

mp911de · 2023-10-18T09:00:49Z

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

@@ -100,18 +102,17 @@ 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();


Introducing a indirection makes it difficult to understand at first sight the details.

Avoiding line breaks and undue noise. Plus, details aren't needed until they are needed. It is easy to navigate into the method.

mp911de · 2023-10-18T09:01:04Z

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

@@ -125,8 +126,7 @@ public class JedisConnectionFactory

 	private @Nullable RedisConfiguration configuration;

-	private RedisStandaloneConfiguration standaloneConfig = new RedisStandaloneConfiguration("localhost",
-			Protocol.DEFAULT_PORT);
+	private RedisStandaloneConfiguration standaloneConfig = defaultStandaloneConfiguration();


Introducing a indirection makes it difficult to understand at first sight the details.

Same reason as above.

mp911de · 2023-10-18T09:02:05Z

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

@@ -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));


Inflationary usage of nullSafe… methods creates a lot of noise hiding the actual details. Introducing indirections makes it difficult to understand at first sight the details.

The lines in question are too busy creating an 1) incomplete/break-in statement and 2) unnecessary line break. They are also redundant.

Plus, "named" methods are more intuitive and this method (nullSafePoolConfig(..)) is reused.

Also, this is not unlike Spring Framework's own nullSafeClassName(..) (Javadoc) and nullSafeConciseToString(..) (Javadoc) methods, or the countless other nullSafe... variants.

Your reference to "indirections" is a bit ironic.

mp911de · 2023-10-18T09:03:52Z

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

 						this.pool = null;
-					} catch (Exception ex) {
-						log.warn("Cannot properly close Jedis pool", ex);
+					} catch (Exception cause) {


To be precise: cause isn't true as the cause refers to the exception cause and not the caught exception. Please stick to ex.

cause is the reason the error occurred in the block above. You are confusing this with caused by.

In any case, I am not going to argue about this.

If you want it to be consistent, then we should call it e everywhere Exceptions are caught (or ignore when Exceptions are not handled), which is what the core Spring Framework uses (i.e. e, not sure about ignore).

Using ex is not consistent, and o_O is not professional.

mp911de · 2023-10-18T09:05:44Z

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

 	static class MutableJedisClientConfiguration implements JedisClientConfiguration {

-		private boolean useSsl;


What's the reason to tear code with good locality apart? All SSL-related fields have been kept together. Now, these are spread across.

Organization by type. All the state variables were bunched together making it difficult to read.

If you want better organization around concern, then perhaps a private, internal SslConfiguration class used by MutableJedisClientConfiguration makes more sense.

Reverted for the time being. This has room for improvement.

Now I see where we get our influence:

package redis.clients.jedis; import ...; public final class DefaultJedisClientConfig implements JedisClientConfig { private final RedisProtocol redisProtocol; private final int connectionTimeoutMillis; private final int socketTimeoutMillis; private final int blockingSocketTimeoutMillis; private volatile Supplier<RedisCredentials> credentialsProvider; private final int database; private final String clientName; private final boolean ssl; private final SSLSocketFactory sslSocketFactory; private final SSLParameters sslParameters; private final HostnameVerifier hostnameVerifier; //...

This is unfortunate. And, this is only marginally better.

mp911de · 2023-10-18T09:09:59Z

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

-			} 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);


Please let's remain on String.format(…) to remain consitent. We can change all occurrences of String.format(…) to formatted if we feel that the value we get from that outweighs the effort.

"some string with placeholders".formatted(..) is less noisy and it makes the "message" more apparent.

Why change try-finally blocks with proper handling to try-with-resources blocks, or use for-each loops when for(index; condition; increment) works perfectly fine?

Simple. Because it reads easier, or in the case of try-with-resources, is a more reliable alternative and welcome language feature.

NOTE: Even multi-line Strings would be a nice addition in certain places where we use really long Exception messages.

I don't believe in big bang, change everything at once and be done with it. All improvements happen incrementally or generally don't/won't happen at all. They are not all or nothing.

In addition, where String formatting, or even concatenation, are concerned, String.format(..) is not even consistently used everywhere, and not just Spring Data Redis.

Case in point: https://github.com/spring-projects/spring-data-redis/blob/3.2.0-RC1/src/main/java/org/springframework/data/redis/core/script/DigestUtils.java#L63

Seems there was already a style in place that predates the existing style. So, either 1) we do not consistently follow any standard or 2) authors "come up with our own way" and then don't consistently apply it everywhere, which enforces my point about "big bang" changes don't work.

Another case in point: https://github.com/spring-projects/spring-data-redis/blob/3.2.0-RC1/src/main/java/org/springframework/data/redis/core/mapping/RedisMappingContext.java#L245-L250

In the RedisMappintContext's case, inconsistent and incorrect message formatting was used.

I am certain there are others.

mp911de · 2023-10-18T09:10:16Z

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

 				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) {


cause is technically wrong, please stick to ex

No. It is the cause for the Exception being thrown in the block above.

mp911de · 2023-10-18T09:11:12Z

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

+	 * @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


No need to create duplicate @see references to types that are already present in the signature.

mp911de · 2023-10-18T09:11:20Z

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

+	 * @see redis.clients.jedis.JedisCluster
+	 * @since 3.2
+	 */
+	protected ClusterCommandExecutor createClusterCommandExecutor(JedisCluster cluster,


No need to create duplicate @see references to types that are already present in the signature.

mp911de · 2023-10-18T09:13:23Z

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

 	 */
+	@Deprecated(since = "3.2")


I do not understand what warrants a deprecation of the createTopologyProvider. New deprecations are a signal for existing users to do something about their usage while we do not solve an actual problem.

API and naming consistency with other cluster-oriented types, along with the type itself, ClusterTopologyProvider.

I was quite surprised when I first learned about Redis that we don't have something equivalent to SentinelTopologyProvider for Redis Sentinel.

Of course, Redis Sentinel is not a fully managed, "clustered" solution (only HA), but is a group of nodes in a distributed system none-the-less.

Technically, ClusterTopology is Set of RedisClusterNode and similarly, RedisSentinelConfiguration contains a Set of RedisNode where RedisClusterNode extends from RedisNode. So, although they are not quite the same, they are similar enough.

Introduces private [static] methods (and/or local variables as needed) to simplify and remove duplicate logic while simultaneously removing broken line breaks affecting readability and understanding. 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. Cleans up compiler warnings. Closes #2745

jxblum · 2023-10-18T23:03:49Z

Did a first review pass. The majority of changes impose a different style, resulting in code that requires more effort to read (harder to read) and that isn't an improvement.

I reworked this PR.

Please keep your comments objective and constructive.

If you re-read the commit message, you will notice several areas where the code was "improved". Cleaning up compiler warnings is a worthwhile improvement all by itself.

Finally, if you don't agree with the deprecation of the "protected" createTopologyProvider(..) (replaced by the new createClusterTopologyProvider(..) method). I don't have strong objections here. I just think it is better, more consistent from naming perspective. Our naming across the API is inconsistent, sometimes referring to the same types, but using different names.

jxblum added type: task A general task in: jedis Jedis driver labels Oct 18, 2023

jxblum added this to the 3.2 GA (2023.1.0) milestone Oct 18, 2023

jxblum force-pushed the main branch from 3679139 to d43adc9 Compare October 18, 2023 02:56

jxblum requested review from christophstrobl and mp911de October 18, 2023 03:04

jxblum force-pushed the main branch from d43adc9 to a71f042 Compare October 18, 2023 03:07

Prepare issue branch for 2745

bdf7fb8

jxblum force-pushed the issue/2745 branch from 6d7d3c1 to 87e02de Compare October 18, 2023 03:12

mp911de requested changes Oct 18, 2023

View reviewed changes

jxblum force-pushed the issue/2745 branch from 87e02de to 972fa5b Compare October 18, 2023 23:04

jxblum changed the title ~~Refine JedisConnectionFactory~~ Refine JedisConnectionFactory Oct 19, 2023

mp911de removed this from the 3.2 GA (2023.1.0) milestone Nov 17, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refine `JedisConnectionFactory` #2746

Refine `JedisConnectionFactory` #2746

jxblum commented Oct 18, 2023

mp911de left a comment

mp911de Oct 18, 2023

jxblum Oct 18, 2023 •

edited

Loading

mp911de Oct 18, 2023

jxblum Oct 18, 2023

mp911de Oct 18, 2023

jxblum Oct 18, 2023 •

edited

Loading

jxblum Oct 18, 2023 •

edited

Loading

mp911de Oct 18, 2023

jxblum Oct 18, 2023

jxblum Oct 18, 2023

mp911de Oct 18, 2023

jxblum Oct 18, 2023

jxblum Oct 18, 2023

jxblum Oct 19, 2023 •

edited

Loading

mp911de Oct 18, 2023

jxblum Oct 18, 2023 •

edited

Loading

jxblum Oct 18, 2023 •

edited

Loading

jxblum Oct 19, 2023 •

edited

Loading

mp911de Oct 18, 2023

jxblum Oct 18, 2023

mp911de Oct 18, 2023

jxblum Oct 18, 2023

mp911de Oct 18, 2023

jxblum Oct 18, 2023

mp911de Oct 18, 2023

jxblum Oct 18, 2023 •

edited

Loading

jxblum Oct 18, 2023

jxblum commented Oct 18, 2023 •

edited

Loading

		static class MutableJedisClientConfiguration implements JedisClientConfiguration {

		private boolean useSsl;

Refine JedisConnectionFactory #2746

Are you sure you want to change the base?

Refine JedisConnectionFactory #2746

Conversation

jxblum commented Oct 18, 2023

mp911de left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jxblum Oct 18, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jxblum Oct 18, 2023 • edited Loading

Choose a reason for hiding this comment

jxblum Oct 18, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jxblum Oct 19, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jxblum Oct 18, 2023 • edited Loading

Choose a reason for hiding this comment

jxblum Oct 18, 2023 • edited Loading

Choose a reason for hiding this comment

jxblum Oct 19, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jxblum Oct 18, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jxblum commented Oct 18, 2023 • edited Loading

Refine `JedisConnectionFactory` #2746

Refine `JedisConnectionFactory` #2746

jxblum Oct 18, 2023 •

edited

Loading

jxblum Oct 18, 2023 •

edited

Loading

jxblum Oct 18, 2023 •

edited

Loading

jxblum Oct 19, 2023 •

edited

Loading

jxblum Oct 18, 2023 •

edited

Loading

jxblum Oct 18, 2023 •

edited

Loading

jxblum Oct 19, 2023 •

edited

Loading

jxblum Oct 18, 2023 •

edited

Loading

jxblum commented Oct 18, 2023 •

edited

Loading