ixti / sidekiq-throttled

Concurrency and rate-limit throttling for Sidekiq
MIT License
698 stars 75 forks source link

fix: DRY-up job class/args extraction #185

Closed ixti closed 5 months ago

ixti commented 5 months ago

Related-PR: https://github.com/ixti/sidekiq-throttled/pull/184

ixti commented 5 months ago

cc: @jlledom

codecov[bot] commented 5 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 98.98%. Comparing base (3c1c543) to head (f6a7b0d).

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #185 +/- ## ========================================== + Coverage 98.95% 98.98% +0.03% ========================================== Files 18 19 +1 Lines 383 396 +13 Branches 55 56 +1 ========================================== + Hits 379 392 +13 Misses 4 4 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

mnovelo commented 5 months ago

Sidekiq::Pro specs pass too!

jeffbax commented 5 months ago

Pardon the crudeness of the example, but I had noticed a similar situation to #184 attempting to adopt this gem over the past week or so (~moving from https://github.com/veeqo/activejob-uniqueness~) although this is for using the threshold strategy not the concurrency one.

ERROR: Failed to get key suffix: wrong number of arguments (given 1, expected 2+)

basically when I substitute

key_suffix: ->(args) do
  puts args
  raise
end

My output is

 {
   "job_class" => "Events::JobKlass", 
   "job_id" => "abaccc09-9916-4ca4-9765-77968d09bbde", 
   "provider_job_id" => nil, 
   "queue_name" => "data_queue_low", 
   "priority" => nil, 
   "arguments" => ["Events::SavedEvent", { "id" => 307, "status_id_changes" => [1, 2], "_aj_symbol_keys" => [] }], 
   "executions" => 0, 
   "exception_executions" => {}, 
   "locale" => "en", 
   "timezone" => "Eastern Time (US & Canada)", 
   "enqueued_at" => "2024-03-28T18:14:14Z", 
   "first_enqueued_at" => 1711649654,
 }

Whereas I was expecting to get the position arguments from the arguments key:

Is this an incorrect assumption on my part or perhaps a similar snag?

ixti commented 5 months ago

Will finish this PR and cut patch release this week.

ixti commented 5 months ago

Released as v1.4.0

jeffbax commented 5 months ago

Released as v1.4.0

thanks! will give it a shot :)