From d023d2fce7ab6c0d3b8859acbb761cbe1002b186 Mon Sep 17 00:00:00 2001 From: M Sazzadul Hoque <7600764+sazzad16@users.noreply.github.com> Date: Thu, 31 Aug 2023 22:50:09 +0600 Subject: [PATCH] Modify JedisBroadcastException (#3518) * Modify JedisBroadcastException * Added tests --- .../exceptions/JedisBroadcastException.java | 34 ++++--------------- .../executors/ClusterCommandExecutor.java | 31 ++++++++--------- .../jedis/ClusterScriptingCommandsTest.java | 18 +++++++++- ...Test.java => PooledMiscellaneousTest.java} | 30 ++++++++++++++-- 4 files changed, 65 insertions(+), 48 deletions(-) rename src/test/java/redis/clients/jedis/commands/unified/pooled/{PooledPipeliningTest.java => PooledMiscellaneousTest.java} (75%) diff --git a/src/main/java/redis/clients/jedis/exceptions/JedisBroadcastException.java b/src/main/java/redis/clients/jedis/exceptions/JedisBroadcastException.java index d0670a2c85..172449c34d 100644 --- a/src/main/java/redis/clients/jedis/exceptions/JedisBroadcastException.java +++ b/src/main/java/redis/clients/jedis/exceptions/JedisBroadcastException.java @@ -1,5 +1,6 @@ package redis.clients.jedis.exceptions; +import java.util.Collections; import java.util.HashMap; import java.util.Map; import redis.clients.jedis.HostAndPort; @@ -8,45 +9,22 @@ * Note: This exception extends {@link JedisDataException} just so existing applications catching * JedisDataException do not get broken. */ +// TODO: extends JedisException public class JedisBroadcastException extends JedisDataException { private static final String BROADCAST_ERROR_MESSAGE = "A failure occurred while broadcasting the command."; - private final Map replies = new HashMap<>(); + private final Map replies = new HashMap<>(); public JedisBroadcastException() { super(BROADCAST_ERROR_MESSAGE); } public void addReply(HostAndPort node, Object reply) { - replies.put(node, new SingleReply(reply)); + replies.put(node, reply); } - public void addError(HostAndPort node, JedisDataException error) { - replies.put(node, new SingleReply(error)); - } - - public static class SingleReply { - - private final Object reply; - private final JedisDataException error; - - public SingleReply(Object reply) { - this.reply = reply; - this.error = null; - } - - public SingleReply(JedisDataException error) { - this.reply = null; - this.error = error; - } - - public Object getReply() { - return reply; - } - - public JedisDataException getError() { - return error; - } + public Map getReplies() { + return Collections.unmodifiableMap(replies); } } diff --git a/src/main/java/redis/clients/jedis/executors/ClusterCommandExecutor.java b/src/main/java/redis/clients/jedis/executors/ClusterCommandExecutor.java index 48ed4cd6a0..4cc25c42a9 100644 --- a/src/main/java/redis/clients/jedis/executors/ClusterCommandExecutor.java +++ b/src/main/java/redis/clients/jedis/executors/ClusterCommandExecutor.java @@ -43,30 +43,29 @@ public final T broadcastCommand(CommandObject commandObject) { boolean isErrored = false; T reply = null; - JedisBroadcastException holder = new JedisBroadcastException(); + JedisBroadcastException bcastError = new JedisBroadcastException(); for (Map.Entry entry : connectionMap.entrySet()) { HostAndPort node = HostAndPort.from(entry.getKey()); ConnectionPool pool = entry.getValue(); try (Connection connection = pool.getResource()) { - try { - T aReply = execute(connection, commandObject); - holder.addReply(node, aReply); - if (isErrored) { // already errored - } else if (reply == null) { - reply = aReply; // ok - } else if (reply.equals(aReply)) { - // ok - } else { - isErrored = true; - reply = null; - } - } catch (JedisDataException anError) { - holder.addError(node, anError); + T aReply = execute(connection, commandObject); + bcastError.addReply(node, aReply); + if (isErrored) { // already errored + } else if (reply == null) { + reply = aReply; // ok + } else if (reply.equals(aReply)) { + // ok + } else { + isErrored = true; + reply = null; } + } catch (Exception anError) { + bcastError.addReply(node, anError); + isErrored = true; } } if (isErrored) { - throw holder; + throw bcastError; } return reply; } diff --git a/src/test/java/redis/clients/jedis/commands/jedis/ClusterScriptingCommandsTest.java b/src/test/java/redis/clients/jedis/commands/jedis/ClusterScriptingCommandsTest.java index a2304bc38c..503337683e 100644 --- a/src/test/java/redis/clients/jedis/commands/jedis/ClusterScriptingCommandsTest.java +++ b/src/test/java/redis/clients/jedis/commands/jedis/ClusterScriptingCommandsTest.java @@ -1,6 +1,8 @@ package redis.clients.jedis.commands.jedis; import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertSame; +import static org.junit.Assert.assertThrows; import static org.junit.Assert.assertTrue; import java.util.ArrayList; @@ -8,10 +10,11 @@ import java.util.Collections; import java.util.List; import java.util.Map; -import java.util.function.Supplier; import org.junit.Test; +import redis.clients.jedis.HostAndPort; import redis.clients.jedis.args.FlushMode; +import redis.clients.jedis.exceptions.JedisBroadcastException; import redis.clients.jedis.exceptions.JedisClusterOperationException; import redis.clients.jedis.exceptions.JedisDataException; @@ -110,4 +113,17 @@ public void broadcast() { assertEquals(Arrays.asList(false, false), cluster.scriptExists(Arrays.asList(sha1_1, sha1_2))); } + + @Test + public void broadcastWithError() { + + JedisBroadcastException error = assertThrows(JedisBroadcastException.class, () -> cluster.functionDelete("xyz")); + + Map replies = error.getReplies(); + assertEquals(3, replies.size()); + replies.values().forEach(r -> { + assertSame(JedisDataException.class, r.getClass()); + assertEquals("ERR Library not found", ((JedisDataException) r).getMessage()); + }); + } } diff --git a/src/test/java/redis/clients/jedis/commands/unified/pooled/PooledPipeliningTest.java b/src/test/java/redis/clients/jedis/commands/unified/pooled/PooledMiscellaneousTest.java similarity index 75% rename from src/test/java/redis/clients/jedis/commands/unified/pooled/PooledPipeliningTest.java rename to src/test/java/redis/clients/jedis/commands/unified/pooled/PooledMiscellaneousTest.java index da2c3f8846..230d9d184b 100644 --- a/src/test/java/redis/clients/jedis/commands/unified/pooled/PooledPipeliningTest.java +++ b/src/test/java/redis/clients/jedis/commands/unified/pooled/PooledMiscellaneousTest.java @@ -1,6 +1,7 @@ package redis.clients.jedis.commands.unified.pooled; import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertThrows; import java.util.ArrayList; import java.util.Arrays; @@ -15,10 +16,10 @@ import redis.clients.jedis.Pipeline; import redis.clients.jedis.Response; import redis.clients.jedis.Transaction; - import redis.clients.jedis.commands.unified.UnifiedJedisCommandsTestBase; +import redis.clients.jedis.exceptions.JedisDataException; -public class PooledPipeliningTest extends UnifiedJedisCommandsTestBase { +public class PooledMiscellaneousTest extends UnifiedJedisCommandsTestBase { protected Pipeline pipeline; protected Transaction transaction; @@ -47,7 +48,7 @@ public void tearDown() { } @Test - public void simple() { + public void pipeline() { final int count = 10; int totalCount = 0; for (int i = 0; i < count; i++) { @@ -104,4 +105,27 @@ public void transaction() { assertEquals(expected.get(i), responses.get(i)); } } + + @Test + public void broadcast() { + + String script_1 = "return 'jedis'"; + String sha1_1 = jedis.scriptLoad(script_1); + + String script_2 = "return 79"; + String sha1_2 = jedis.scriptLoad(script_2); + + assertEquals(Arrays.asList(true, true), jedis.scriptExists(Arrays.asList(sha1_1, sha1_2))); + + jedis.scriptFlush(); + + assertEquals(Arrays.asList(false, false), jedis.scriptExists(Arrays.asList(sha1_1, sha1_2))); + } + + @Test + public void broadcastWithError() { + JedisDataException error = assertThrows(JedisDataException.class, + () -> jedis.functionDelete("xyz")); + assertEquals("ERR Library not found", error.getMessage()); + } }