remove() may return 'false' even though it removed the element

remove() was implemented incorrectly. This fixes it.
Netflix · Sep 17, 2019 · 52f79ee · 52f79ee
1 parent 761e231
commit 52f79ee
Show file tree

Hide file tree

Showing 2 changed files with 10 additions and 2 deletions.
diff --git a/dyno-queues-redis/src/main/java/com/netflix/dyno/queues/demo/DynoQueueDemo.java b/dyno-queues-redis/src/main/java/com/netflix/dyno/queues/demo/DynoQueueDemo.java
@@ -89,6 +89,9 @@ private void runSimpleV1Demo(DynoJedisClient dyno) throws IOException {
         payloads.add(new Message("id4", "payload 4"));
         payloads.add(new Message("id5", "payload 5"));
         payloads.add(new Message("id6", "payload 6"));
+        payloads.add(new Message("id7", "payload 7"));
+        payloads.add(new Message("id8", "payload 8"));
+        payloads.add(new Message("id9", "payload 9"));
         DynoQueue V1Queue = queues.get("simpleQueue");
 
         // Clear the queue in case the server already has the above key.
@@ -121,6 +124,10 @@ private void runSimpleV1Demo(DynoJedisClient dyno) throws IOException {
         boolean ack_successful = V1Queue.ack(specific_pops.get(0).getId());
         assert(ack_successful);
 
+        // Test remove()
+        boolean removed = V1Queue.remove("id9");
+        assert(removed);
+
         // Test pop(). Even though we try to pop 3 messages, there will only be one remaining message in our local shard.
         List<Message> popped_msgs = V1Queue.pop(3, 1000, TimeUnit.MILLISECONDS);
         V1Queue.ack(popped_msgs.get(0).getId());

diff --git a/dyno-queues-redis/src/main/java/com/netflix/dyno/queues/redis/RedisDynoQueue.java b/dyno-queues-redis/src/main/java/com/netflix/dyno/queues/redis/RedisDynoQueue.java
@@ -458,9 +458,10 @@ public boolean remove(String messageId) {
 
                     String queueShardKey = getQueueShardKey(queueName, shard);
                     Long removed = quorumConn.zrem(queueShardKey, messageId);
-                    Long msgRemoved = quorumConn.hdel(messageStoreKey, messageId);
 
-                    if (removed > 0 && msgRemoved > 0) {
+                    if (removed > 0) {
+                        // Ignoring return value since we just want to get rid of it.
+                        Long msgRemoved = quorumConn.hdel(messageStoreKey, messageId);
                         return true;
                     }
                 }