Merge pull request #2473 from pik/develop

Do not perform redundant job look-up on delete for SortedEntry

lib/sidekiq/api.rb

@@ -369,11 +369,15 @@ module Sidekiq
     end
 
     def delete
-      @parent.delete(score, jid)
+      if @value
+        @parent.delete_by_value(@parent.name, @value)
+      else
+        @parent.delete_by_jid(score, jid)
+      end
     end
 
     def reschedule(at)
-      @parent.delete(score, jid)
+      delete
       @parent.schedule(at, item)
     end
 
@@ -519,36 +523,30 @@ module Sidekiq
       self.detect { |j| j.jid == jid }
     end
 
-    def delete(score, jid = nil)
-      if jid
-        elements = Sidekiq.redis do |conn|
-          conn.zrangebyscore(name, score, score)
-        end
-
-        elements_with_jid = elements.map do |element|
-          message = Sidekiq.load_json(element)
-
-          if message["jid"] == jid
-            _, @_size = Sidekiq.redis do |conn|
-              conn.multi do
-                conn.zrem(name, element)
-                conn.zcard name
-              end
-            end
-          end
-        end
-        elements_with_jid.count != 0
-      else
-        count, @_size = Sidekiq.redis do |conn|
-          conn.multi do
-            conn.zremrangebyscore(name, score, score)
-            conn.zcard name
-          end
-        end
-        count != 0
+    def delete_by_value(name, value)
+      Sidekiq.redis do |conn|
+        ret = conn.zrem(name, value)
+        @_size -= 1 if ret
+        ret
       end
     end
 
+    def delete_by_jid(score, jid)
+      Sidekiq.redis do |conn|
+        elements = conn.zrangebyscore(name, score, score)
+        elements.each do |element|
+          message = Sidekiq.load_json(element)
+          if message["jid"] == jid
+            ret = conn.zrem(name, element)
+            @_size -= 1 if ret
+            break ret
+          end
+          false
+        end
+      end
+    end
+
+    alias_method :delete, :delete_by_jid
   end
 
   ##

test/test_api.rb

@@ -346,14 +346,12 @@ class TestApi < Sidekiq::Test
       assert_in_delta Time.now.to_f, retri.at.to_f, 0.02
     end
 
-    it 'can delete multiple retries from score' do
-      same_time = Time.now.to_f
-      add_retry('bob1', same_time)
-      add_retry('bob2', same_time)
-      r = Sidekiq::RetrySet.new
-      assert_equal 2, r.size
-      Sidekiq::RetrySet.new.delete(same_time)
-      assert_equal 0, r.size
+    it 'requires a jid to delete an entry' do
+      start_time = Time.now.to_f
+      add_retry('bob2', Time.now.to_f)
+      assert_raises(ArgumentError) do
+        Sidekiq::RetrySet.new.delete(start_time)
+      end
     end
 
     it 'can delete a single retry from score and jid' do