From 31f19b04b7af68a3b3b482f48f10f4aeac61c124 Mon Sep 17 00:00:00 2001 From: Steve Dierker Date: Thu, 27 Apr 2023 18:14:35 +0200 Subject: [PATCH 1/2] Because `replace` is a client strategy, it should only remove client locks aka queue locks. --- lib/sidekiq_unique_jobs/digests.rb | 28 +++++++++++++------ .../on_conflict/replace.rb | 2 +- 2 files changed, 20 insertions(+), 10 deletions(-) diff --git a/lib/sidekiq_unique_jobs/digests.rb b/lib/sidekiq_unique_jobs/digests.rb index e10e371d5..133bd059b 100644 --- a/lib/sidekiq_unique_jobs/digests.rb +++ b/lib/sidekiq_unique_jobs/digests.rb @@ -41,17 +41,27 @@ def delete_by_pattern(pattern, count: DEFAULT_COUNT) # Also deletes the :AVAILABLE, :EXPIRED etc keys # # @param [String] digest a unique digest to delete - def delete_by_digest(digest) # rubocop:disable Metrics/MethodLength + # @param queuetime [Boolean] Whether to delete queue locks. + # @param runtime [Boolean] Whether to delete run locks. + def delete_by_digest(digest, queuetime: true, runtime: true) # rubocop:disable Metrics/MethodLength result, elapsed = timed do + queue = queuetime ? digest : "" + queue_queued = queuetime ? "#{digest}:QUEUED" : "" + queue_primed = queuetime ? "#{digest}:PRIMED" : "" + queue_locked = queuetime ? "#{digest}:LOCKED" : "" + run = runtime ? "#{digest}:RUN" : "" + run_queued = runtime ? "#{digest}:RUN:QUEUED" : "" + run_primed = runtime ? "#{digest}:RUN:PRIMED" : "" + run_locked = runtime ? "#{digest}:RUN:LOCKED" : "" call_script(:delete_by_digest, [ - digest, - "#{digest}:QUEUED", - "#{digest}:PRIMED", - "#{digest}:LOCKED", - "#{digest}:RUN", - "#{digest}:RUN:QUEUED", - "#{digest}:RUN:PRIMED", - "#{digest}:RUN:LOCKED", + queue, + queue_queued, + queue_primed, + queue_locked, + run, + run_queued, + run_primed, + run_locked, key, ]) end diff --git a/lib/sidekiq_unique_jobs/on_conflict/replace.rb b/lib/sidekiq_unique_jobs/on_conflict/replace.rb index d434da9e7..2c413085c 100644 --- a/lib/sidekiq_unique_jobs/on_conflict/replace.rb +++ b/lib/sidekiq_unique_jobs/on_conflict/replace.rb @@ -65,7 +65,7 @@ def delete_job_by_digest # @return [Integer] the number of keys deleted # def delete_lock - digests.delete_by_digest(lock_digest) + digests.delete_by_digest(lock_digest, runtime: false) end # From 96939cebf61e2ffc3e3035d2af5e7220fba3e1b1 Mon Sep 17 00:00:00 2001 From: Steve Dierker Date: Thu, 27 Jul 2023 16:00:57 +0200 Subject: [PATCH 2/2] Imrove code quality and reduce method size. Cleaning up `Digests#delete_by_digest`. Instead of assembling all keys inside of the method, we introduced a const and two helper methods to make the code easier on the eyes. --- lib/sidekiq_unique_jobs/digests.rb | 63 ++++++++++++++++++++---------- 1 file changed, 43 insertions(+), 20 deletions(-) diff --git a/lib/sidekiq_unique_jobs/digests.rb b/lib/sidekiq_unique_jobs/digests.rb index 133bd059b..39d840969 100644 --- a/lib/sidekiq_unique_jobs/digests.rb +++ b/lib/sidekiq_unique_jobs/digests.rb @@ -7,6 +7,16 @@ module SidekiqUniqueJobs # @author Mikael Henriksson # class Digests < Redis::SortedSet + # + # @return [Integer] the number of matches to return by default + DEFAULT_COUNT = 1_000 + # + # @return [String] the default pattern to use for matching + SCAN_PATTERN = "*" + # + # @return [Array(String, String, String, String)] The empty runtime or queuetime keys. + EMPTY_KEYS_SEGMENT = ["", "", "", ""].freeze + def initialize(digests_key = DIGESTS) super(digests_key) end @@ -43,27 +53,12 @@ def delete_by_pattern(pattern, count: DEFAULT_COUNT) # @param [String] digest a unique digest to delete # @param queuetime [Boolean] Whether to delete queue locks. # @param runtime [Boolean] Whether to delete run locks. - def delete_by_digest(digest, queuetime: true, runtime: true) # rubocop:disable Metrics/MethodLength + def delete_by_digest(digest, queuetime: true, runtime: true) result, elapsed = timed do - queue = queuetime ? digest : "" - queue_queued = queuetime ? "#{digest}:QUEUED" : "" - queue_primed = queuetime ? "#{digest}:PRIMED" : "" - queue_locked = queuetime ? "#{digest}:LOCKED" : "" - run = runtime ? "#{digest}:RUN" : "" - run_queued = runtime ? "#{digest}:RUN:QUEUED" : "" - run_primed = runtime ? "#{digest}:RUN:PRIMED" : "" - run_locked = runtime ? "#{digest}:RUN:LOCKED" : "" - call_script(:delete_by_digest, [ - queue, - queue_queued, - queue_primed, - queue_locked, - run, - run_queued, - run_primed, - run_locked, - key, - ]) + call_script( + :delete_by_digest, + queuetime_keys(queuetime ? digest : nil) + runtime_keys(runtime ? digest : nil) + [key], + ) end log_info("#{__method__}(#{digest}) completed in #{elapsed}ms") @@ -107,5 +102,33 @@ def page(cursor: 0, pattern: SCAN_PATTERN, page_size: 100) ] end end + + private + + # @param digest [String, nil] The digest to form runtime keys from. + # @return [Array(String, String, String, String)] The list of runtime keys or empty strings if +digest+ was +nil+. + def runtime_keys(digest) + return EMPTY_KEYS_SEGMENT unless digest + + [ + "#{digest}:RUN", + "#{digest}:RUN:QUEUED", + "#{digest}:RUN:PRIMED", + "#{digest}:RUN:LOCKED", + ] + end + + # @param digest [String, nil] The digest to form queuetime keys from. + # @return [Array(String, String, String, String)] The list of queuetime keys or empty strings if +digest+ was +nil+. + def queuetime_keys(digest) + return EMPTY_KEYS_SEGMENT unless digest + + [ + digest, + "#{digest}:QUEUED", + "#{digest}:PRIMED", + "#{digest}:LOCKED", + ] + end end end