From 0f6a152295ae28d9253cc2d711b14200aca69cd4 Mon Sep 17 00:00:00 2001 From: Mark Paluch Date: Mon, 29 Mar 2021 11:39:39 +0200 Subject: [PATCH] Polishing. Reorder methods. Move rewriteConfig directly implemented on connection to DefaultedRedisConnection respective DefaultedRedisClusterConnection. Add overload to run config rewrite on individual cluster nodes. Add tests. Add since tags and update Javadoc. See #1992 Original pull request: #2012. --- .../DefaultStringRedisConnection.java | 14 +++++++---- .../DefaultedRedisClusterConnection.java | 7 ++++++ .../connection/DefaultedRedisConnection.java | 7 ++++++ .../RedisClusterServerCommands.java | 7 ++++++ .../redis/connection/RedisServerCommands.java | 12 ++++++--- .../jedis/JedisClusterServerCommands.java | 25 ++++++++++++++----- .../connection/jedis/JedisConnection.java | 4 --- .../connection/jedis/JedisServerCommands.java | 14 +++++++---- .../lettuce/LettuceClusterServerCommands.java | 18 +++++++++++++ .../connection/lettuce/LettuceConnection.java | 5 ---- .../lettuce/LettuceServerCommands.java | 14 +++++++---- .../JedisClusterConnectionUnitTests.java | 21 ++++++++++++++++ .../LettuceClusterConnectionUnitTests.java | 20 +++++++++++++++ 13 files changed, 134 insertions(+), 34 deletions(-) diff --git a/src/main/java/org/springframework/data/redis/connection/DefaultStringRedisConnection.java b/src/main/java/org/springframework/data/redis/connection/DefaultStringRedisConnection.java index d85d8dedd..89820ee1e 100644 --- a/src/main/java/org/springframework/data/redis/connection/DefaultStringRedisConnection.java +++ b/src/main/java/org/springframework/data/redis/connection/DefaultStringRedisConnection.java @@ -878,6 +878,15 @@ public class DefaultStringRedisConnection implements StringRedisConnection, Deco delegate.resetConfigStats(); } + /* + * (non-Javadoc) + * @see org.springframework.data.redis.connection.RedisServerCommands#rewriteConfig() + */ + @Override + public void rewriteConfig() { + delegate.rewriteConfig(); + } + /* * (non-Javadoc) * @see org.springframework.data.redis.connection.RedisListCommands#rPop(byte[]) @@ -3731,11 +3740,6 @@ public class DefaultStringRedisConnection implements StringRedisConnection, Deco delegate.migrate(key, target, dbIndex, option, timeout); } - @Override - public void rewriteConfig() { - delegate.rewriteConfig(); - } - /* * (non-Javadoc) * @see org.springframework.data.redis.connection.StringRedisConnection#xAck(java.lang.String, java.lang.String, RecordId[]) diff --git a/src/main/java/org/springframework/data/redis/connection/DefaultedRedisClusterConnection.java b/src/main/java/org/springframework/data/redis/connection/DefaultedRedisClusterConnection.java index 7678ad483..878f89803 100644 --- a/src/main/java/org/springframework/data/redis/connection/DefaultedRedisClusterConnection.java +++ b/src/main/java/org/springframework/data/redis/connection/DefaultedRedisClusterConnection.java @@ -122,6 +122,13 @@ public interface DefaultedRedisClusterConnection extends RedisClusterConnection, serverCommands().resetConfigStats(node); } + /** @deprecated in favor of {@link RedisConnection#serverCommands()}. */ + @Override + @Deprecated + default void rewriteConfig(RedisClusterNode node) { + serverCommands().rewriteConfig(node); + } + /** @deprecated in favor of {@link RedisConnection#serverCommands()}. */ @Override @Deprecated diff --git a/src/main/java/org/springframework/data/redis/connection/DefaultedRedisConnection.java b/src/main/java/org/springframework/data/redis/connection/DefaultedRedisConnection.java index 76ff0e4e0..617863499 100644 --- a/src/main/java/org/springframework/data/redis/connection/DefaultedRedisConnection.java +++ b/src/main/java/org/springframework/data/redis/connection/DefaultedRedisConnection.java @@ -1430,6 +1430,13 @@ public interface DefaultedRedisConnection extends RedisConnection { serverCommands().resetConfigStats(); } + /** @deprecated in favor of {@link RedisConnection#serverCommands()}. */ + @Override + @Deprecated + default void rewriteConfig() { + serverCommands().rewriteConfig(); + } + /** @deprecated in favor of {@link RedisConnection#serverCommands()}. */ @Override @Deprecated diff --git a/src/main/java/org/springframework/data/redis/connection/RedisClusterServerCommands.java b/src/main/java/org/springframework/data/redis/connection/RedisClusterServerCommands.java index 8e790ed64..70bdcd687 100644 --- a/src/main/java/org/springframework/data/redis/connection/RedisClusterServerCommands.java +++ b/src/main/java/org/springframework/data/redis/connection/RedisClusterServerCommands.java @@ -114,6 +114,13 @@ public interface RedisClusterServerCommands extends RedisServerCommands { */ void resetConfigStats(RedisClusterNode node); + /** + * @param node must not be {@literal null}. + * @see RedisServerCommands#rewriteConfig() + * @since 2.5 + */ + void rewriteConfig(RedisClusterNode node); + /** * @param node must not be {@literal null}. * @return diff --git a/src/main/java/org/springframework/data/redis/connection/RedisServerCommands.java b/src/main/java/org/springframework/data/redis/connection/RedisServerCommands.java index 799b5e0e2..83b2378cb 100644 --- a/src/main/java/org/springframework/data/redis/connection/RedisServerCommands.java +++ b/src/main/java/org/springframework/data/redis/connection/RedisServerCommands.java @@ -174,6 +174,14 @@ public interface RedisServerCommands { */ void resetConfigStats(); + /** + * Rewrites the {@code redis.conf} file. + * + * @since 2.5 + * @see Redis Documentation: CONFIG REWRITE + */ + void rewriteConfig(); + /** * Request server timestamp using {@code TIME} command in {@link TimeUnit#MILLISECONDS}. * @@ -281,8 +289,4 @@ public interface RedisServerCommands { */ void migrate(byte[] key, RedisNode target, int dbIndex, @Nullable MigrateOption option, long timeout); - /** - * Rewrites the redis.conf file. - */ - void rewriteConfig(); } diff --git a/src/main/java/org/springframework/data/redis/connection/jedis/JedisClusterServerCommands.java b/src/main/java/org/springframework/data/redis/connection/jedis/JedisClusterServerCommands.java index 075f9feed..d88f60b09 100644 --- a/src/main/java/org/springframework/data/redis/connection/jedis/JedisClusterServerCommands.java +++ b/src/main/java/org/springframework/data/redis/connection/jedis/JedisClusterServerCommands.java @@ -381,6 +381,16 @@ class JedisClusterServerCommands implements RedisClusterServerCommands { .executeCommandOnAllNodes((JedisClusterCommandCallback) BinaryJedis::configResetStat); } + /* + * (non-Javadoc) + * @see org.springframework.data.redis.connection.RedisServerCommands#rewriteConfig() + */ + @Override + public void rewriteConfig() { + connection.getClusterCommandExecutor() + .executeCommandOnAllNodes((JedisClusterCommandCallback) BinaryJedis::configRewrite); + } + /* * (non-Javadoc) * @see org.springframework.data.redis.connection.RedisClusterServerCommands#resetConfigStats(org.springframework.data.redis.connection.RedisClusterNode) @@ -390,6 +400,15 @@ class JedisClusterServerCommands implements RedisClusterServerCommands { executeCommandOnSingleNode(BinaryJedis::configResetStat, node); } + /* + * (non-Javadoc) + * @see org.springframework.data.redis.connection.RedisClusterServerCommands#rewriteConfig(org.springframework.data.redis.connection.RedisClusterNode) + */ + @Override + public void rewriteConfig(RedisClusterNode node) { + executeCommandOnSingleNode(BinaryJedis::configRewrite, node); + } + /* * (non-Javadoc) * @see org.springframework.data.redis.connection.RedisServerCommands#time(TimeUnit) @@ -520,12 +539,6 @@ class JedisClusterServerCommands implements RedisClusterServerCommands { node); } - @Override - public void rewriteConfig() { - connection.getClusterCommandExecutor() - .executeCommandOnAllNodes((JedisClusterCommandCallback) client -> client.configRewrite()); - } - private Long convertListOfStringToTime(List serverTimeInformation, TimeUnit timeUnit) { Assert.notEmpty(serverTimeInformation, "Received invalid result from server. Expected 2 items in collection."); diff --git a/src/main/java/org/springframework/data/redis/connection/jedis/JedisConnection.java b/src/main/java/org/springframework/data/redis/connection/jedis/JedisConnection.java index d0f11227c..22ab9a0b3 100644 --- a/src/main/java/org/springframework/data/redis/connection/jedis/JedisConnection.java +++ b/src/main/java/org/springframework/data/redis/connection/jedis/JedisConnection.java @@ -806,8 +806,4 @@ public class JedisConnection extends AbstractRedisConnection { } } - @Override - public void rewriteConfig() { - serverCommands().rewriteConfig(); - } } diff --git a/src/main/java/org/springframework/data/redis/connection/jedis/JedisServerCommands.java b/src/main/java/org/springframework/data/redis/connection/jedis/JedisServerCommands.java index b8c0c319a..99e83bc6c 100644 --- a/src/main/java/org/springframework/data/redis/connection/jedis/JedisServerCommands.java +++ b/src/main/java/org/springframework/data/redis/connection/jedis/JedisServerCommands.java @@ -189,6 +189,15 @@ class JedisServerCommands implements RedisServerCommands { connection.invokeStatus().just(BinaryJedis::configResetStat, MultiKeyPipelineBase::configResetStat); } + /* + * (non-Javadoc) + * @see org.springframework.data.redis.connection.RedisServerCommands#resetConfigStats() + */ + @Override + public void rewriteConfig() { + connection.invokeStatus().just(Jedis::configRewrite); + } + /* * (non-Javadoc) * @see org.springframework.data.redis.connection.RedisServerCommands#time(TimeUnit) @@ -317,11 +326,6 @@ class JedisServerCommands implements RedisServerCommands { target.getPort(), key, dbIndex, timeoutToUse); } - @Override - public void rewriteConfig() { - connection.invokeStatus().just(Jedis::configRewrite); - } - private boolean isPipelined() { return connection.isPipelined(); } diff --git a/src/main/java/org/springframework/data/redis/connection/lettuce/LettuceClusterServerCommands.java b/src/main/java/org/springframework/data/redis/connection/lettuce/LettuceClusterServerCommands.java index 1b1a36013..39aba2b7d 100644 --- a/src/main/java/org/springframework/data/redis/connection/lettuce/LettuceClusterServerCommands.java +++ b/src/main/java/org/springframework/data/redis/connection/lettuce/LettuceClusterServerCommands.java @@ -307,6 +307,24 @@ class LettuceClusterServerCommands extends LettuceServerCommands implements Redi executeCommandOnSingleNode(RedisServerCommands::configResetstat, node); } + /* + * (non-Javadoc) + * @see org.springframework.data.redis.connection.lettuce.LettuceServerCommands#rewriteConfig() + */ + @Override + public void rewriteConfig() { + executeCommandOnAllNodes(RedisServerCommands::configRewrite); + } + + /* + * (non-Javadoc) + * @see org.springframework.data.redis.connection.RedisClusterServerCommands#rewriteConfig(org.springframework.data.redis.connection.RedisClusterNode) + */ + @Override + public void rewriteConfig(RedisClusterNode node) { + executeCommandOnSingleNode(RedisServerCommands::configRewrite, node); + } + /* * (non-Javadoc) * @see org.springframework.data.redis.connection.RedisClusterServerCommands#time(org.springframework.data.redis.connection.RedisClusterNode) diff --git a/src/main/java/org/springframework/data/redis/connection/lettuce/LettuceConnection.java b/src/main/java/org/springframework/data/redis/connection/lettuce/LettuceConnection.java index f8eff3c15..708344cc2 100644 --- a/src/main/java/org/springframework/data/redis/connection/lettuce/LettuceConnection.java +++ b/src/main/java/org/springframework/data/redis/connection/lettuce/LettuceConnection.java @@ -137,11 +137,6 @@ public class LettuceConnection extends AbstractRedisConnection { return LettuceResultBuilder. forResponse(resultHolder).buildStatusResult(); } - @Override - public void rewriteConfig() { - serverCommands().rewriteConfig(); - } - private class LettuceTransactionResultConverter extends TransactionResultConverter { public LettuceTransactionResultConverter(Queue> txResults, Converter exceptionConverter) { diff --git a/src/main/java/org/springframework/data/redis/connection/lettuce/LettuceServerCommands.java b/src/main/java/org/springframework/data/redis/connection/lettuce/LettuceServerCommands.java index 5294e0d8e..0e13eb87b 100644 --- a/src/main/java/org/springframework/data/redis/connection/lettuce/LettuceServerCommands.java +++ b/src/main/java/org/springframework/data/redis/connection/lettuce/LettuceServerCommands.java @@ -199,6 +199,15 @@ class LettuceServerCommands implements RedisServerCommands { connection.invokeStatus().just(RedisServerAsyncCommands::configResetstat); } + /* + * (non-Javadoc) + * @see org.springframework.data.redis.connection.RedisServerCommands#resetConfigStats() + */ + @Override + public void rewriteConfig() { + connection.invokeStatus().just(RedisServerAsyncCommands::configRewrite); + } + /* * (non-Javadoc) * @see org.springframework.data.redis.connection.RedisServerCommands#time(TimeUnit) @@ -304,11 +313,6 @@ class LettuceServerCommands implements RedisServerCommands { connection.invoke().just(RedisKeyAsyncCommands::migrate, target.getHost(), target.getPort(), key, dbIndex, timeout); } - @Override - public void rewriteConfig() { - connection.invoke().just(RedisServerAsyncCommands::configRewrite); - } - public RedisClusterCommands getConnection() { return connection.getConnection(); } diff --git a/src/test/java/org/springframework/data/redis/connection/jedis/JedisClusterConnectionUnitTests.java b/src/test/java/org/springframework/data/redis/connection/jedis/JedisClusterConnectionUnitTests.java index 0fdbf19e4..d49ef6850 100644 --- a/src/test/java/org/springframework/data/redis/connection/jedis/JedisClusterConnectionUnitTests.java +++ b/src/test/java/org/springframework/data/redis/connection/jedis/JedisClusterConnectionUnitTests.java @@ -333,6 +333,27 @@ class JedisClusterConnectionUnitTests { verify(con3Mock, never()).configResetStat(); } + @Test // GH-1992 + void rewriteConfigShouldBeExecutedOnAllNodes() { + + connection.rewriteConfig(); + + verify(con1Mock, times(1)).configRewrite(); + verify(con2Mock, times(1)).configRewrite(); + verify(con3Mock, times(1)).configRewrite(); + } + + @Test // GH-1992 + void rewriteConfigShouldBeExecutedOnSingleNodeCorrectly() { + + connection.rewriteConfig(CLUSTER_NODE_2); + + verify(con2Mock, times(1)).configRewrite(); + verify(con2Mock, atLeast(1)).close(); + verify(con1Mock, never()).configRewrite(); + verify(con3Mock, never()).configRewrite(); + } + @Test // DATAREDIS-315 void clusterTopologyProviderShouldCollectErrorsWhenLoadingNodes() { diff --git a/src/test/java/org/springframework/data/redis/connection/lettuce/LettuceClusterConnectionUnitTests.java b/src/test/java/org/springframework/data/redis/connection/lettuce/LettuceClusterConnectionUnitTests.java index ee10e7ddc..10c440e20 100644 --- a/src/test/java/org/springframework/data/redis/connection/lettuce/LettuceClusterConnectionUnitTests.java +++ b/src/test/java/org/springframework/data/redis/connection/lettuce/LettuceClusterConnectionUnitTests.java @@ -357,6 +357,26 @@ class LettuceClusterConnectionUnitTests { verify(clusterConnection1Mock, never()).configResetstat(); } + @Test // GH-1992 + void rewriteConfigShouldBeExecutedOnAllNodes() { + + connection.rewriteConfig(); + + verify(clusterConnection1Mock, times(1)).configRewrite(); + verify(clusterConnection2Mock, times(1)).configRewrite(); + verify(clusterConnection3Mock, times(1)).configRewrite(); + } + + @Test // GH-1992 + void rewriteConfigShouldBeExecutedOnSingleNodeCorrectly() { + + connection.rewriteConfig(CLUSTER_NODE_2); + + verify(clusterConnection2Mock, times(1)).configRewrite(); + verify(clusterConnection1Mock, never()).configRewrite(); + verify(clusterConnection1Mock, never()).configRewrite(); + } + @Test // DATAREDIS-731, DATAREDIS-545 void shouldExecuteOnSharedConnection() {