Modify JedisBroadcastException (#3518)

* Modify JedisBroadcastException * Added tests
redis · Aug 31, 2023 · 3d45faf · 3d45faf
1 parent d10a7fc
commit 3d45faf
Show file tree

Hide file tree

Showing 4 changed files with 65 additions and 48 deletions.
diff --git 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<HostAndPort, SingleReply> replies = new HashMap<>();
+  private final Map<HostAndPort, Object> 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<HostAndPort, Object> 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
@@ -43,30 +43,29 @@ public final <T> T broadcastCommand(CommandObject<T> commandObject) {
 
     boolean isErrored = false;
     T reply = null;
-    JedisBroadcastException holder = new JedisBroadcastException();
+    JedisBroadcastException bcastError = new JedisBroadcastException();
     for (Map.Entry<String, ConnectionPool> 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
@@ -1,17 +1,20 @@
 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;
 import java.util.Arrays;
 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<HostAndPort, Object> 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/.../unified/pooled/PooledPipeliningTest.java → ...ified/pooled/PooledMiscellaneousTest.java b/.../unified/pooled/PooledPipeliningTest.java → ...ified/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());
+  }
 }