Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use job payload instead of worker instance in middleware #33

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

tony-pizza
Copy link

I'm proposing that we drive the Sidekiq middleware off of the job payload hash instead of the worker instance.

Why?

There are many ways to configure job options on a Sidekiq worker (See sidekiq/sidekiq#6092 (comment)):

Sidekiq.default_job_options
sidekiq_options key: value
JobClass.set(key: value)
PeriodicManager.register(..., key: value)

Sidekiq merges all of these options into one payload that gets passed into middleware (as the second argument of call()).

In the current middleware, job options are mostly fetched via worker.class.sidekiq_options, which would only look at options set via sidekiq_options in the worker class and ignore any options set via other methods (e.g. periodic job config). Addressing this by fetching options from various places (as is done in periodic_job_key()) adds unnecessary work and complexity as Sidekiq already merges options from various sources into the job payload.

What?

  • Refactors the internals of the Sidekiq server middleware to use the job payload hash instead of the worker instance
  • Updates specs accordingly

Comment on lines -48 to -56
def periodic_job_key(worker)
return unless defined?(Sidekiq::Periodic)

periodic_job = Sidekiq::Periodic::LoopSet.new.find do |lop|
lop.history.find { |j| j[0] == worker.jid }
end

periodic_job.present? && periodic_job.options.fetch('cronitor_key', nil)
end
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need for this at all anymore :)

Comment on lines -62 to -63
# symbolize_keys is a rails helper, so only use it if it's defined
opts = opts.symbolize_keys if opts.respond_to?(:symbolize_keys)
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I removed this as usage outside of Rails would have led to bugs. We can expect job_payload to always have string keys.

lop.history.find { |j| j[0] == worker.jid }
end

periodic_job.present? && periodic_job.options.fetch('cronitor_key', nil)
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, I'm pretty sure periodic_job.options.fetch() would raise an error anyway. Same issue as #31

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant