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.

Sign up for GitHub

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

Jump to bottom

Correlated async handlers #379

Merged
merged 8 commits into from
Jun 25, 2018
Merged

Correlated async handlers #379

merged 8 commits into from
Jun 25, 2018

Conversation

paneq
Copy link
Member

@paneq paneq commented Jun 22, 2018

  • documentation

@paneq paneq self-assigned this Jun 22, 2018
@paneq
Copy link
Member Author

paneq commented Jun 22, 2018

for review after #375

paneq added 3 commits June 25, 2018 10:49
@paneq
Use inline adapter when async adapter not available
54f321f 
in Rails 4.2.*

It doesn't really test much (which was revealed by mutation testing and
thus changed to :async) because it executes the job in the same thread
and thus getting the metadata that we prepared for sync-handlers. So not
really testing what we have in mind.

We should include additional integration tests which would verify with
a few other adapters for safety.

Issue: #346
@paneq
Merge remote-tracking branch 'origin/master' into correlated_async_ha…
13ce45d 
…ndlers
@paneq
Test with sidekiq
9c13f13 
to be double sure that our serialization for async handlers works
properly.

Issue: #346

----- ActiveJob::Base.queue_adapter global -----

Unfortunately in rails 4.2 Active job queue adapter was a class variable
shared between all classes:

module ActiveJob
  module QueueAdapter #:nodoc:
    extend ActiveSupport::Concern
    module ClassMethods
      mattr_reader(:queue_adapter) { ActiveJob::QueueAdapters::InlineAdapter }
      def queue_adapter=(name_or_adapter)
        @@queue_adapter = \
          case name_or_adapter
          when :test
            ActiveJob::QueueAdapters::TestAdapter.new
          when Symbol, String
            load_adapter(name_or_adapter)
          else
            name_or_adapter if name_or_adapter.respond_to?(:enqueue)
          end
      end

as a result writing:

class MySidekiqHandler < ActiveJob::Base
  self.queue_adapter = :sidekiq

affected all tests :( and set the queue adapter to sidekiq for all
handlers and not this one particular only.

Rails 5.2 uses class_attribute from ActiveSupport:
* Declare a class-level attribute whose value is inheritable by subclasses.
* Subclasses can change their own value and it will not impact parent class.

but we need our tests to work on both versions right now.

So I decided to operate on ActiveJob::Base.queue_adapter in all test to
unify the behavior between all versions that we support.

----- Sidekiq testing -----

I did not plan originally to use Sidekiq::Testing as I wanted be as close
to real scenario as possible. But I could not find an easy way to run a
real worker that would get the jobs from redis and process them and then
quit.

This seems to be good enough as we go through the serialization process:

  class Client
    alias_method :raw_push_real, :raw_push

    def raw_push(payloads)
      if Sidekiq::Testing.fake?
        payloads.each do |job|
          job = Sidekiq.load_json(Sidekiq.dump_json(job))
          job.merge!('enqueued_at' => Time.now.to_f) unless job['at']
          Queues.push(job['queue'], job['class'], job)
        end
        true
      elsif Sidekiq::Testing.inline?
        payloads.each do |job|
          klass = Sidekiq::Testing.constantize(job['class'])
          job['id'] ||= SecureRandom.hex(12)
          job_hash = Sidekiq.load_json(Sidekiq.dump_json(job))
          klass.process_job(job_hash)
        end
        true
      else
        raw_push_real(payloads)
      end
    end
  end

I run Sidekiq::Worker.drain_all in a new thread to decouple from
thread-level global variables.

https://github.com/mperham/sidekiq/wiki/Testing

----- STDOUT -----

Sidekiq is unhappy about Rails.env when we require it:

if defined?(::Rails) && Rails.respond_to?(:env) && !Rails.env.test?
  puts("⛔️ WARNING: Sidekiq testing API enabled...")

I reassign $stdout to mute this warning which is not of value for us.
@paneq paneq merged commit 39db4f6 into master Jun 25, 2018
@paneq paneq deleted the correlated_async_handlers branch June 25, 2018 14:54
@paneq paneq added this to the v0.31 milestone Jun 25, 2018
@joelvh
Copy link
Contributor

joelvh commented Jun 25, 2018

How about for non-Rails environments?

@paneq
Copy link
Member Author

paneq commented Jun 25, 2018

@joelvh invoke correlate_with

@joelvh
Copy link
Contributor

joelvh commented Jun 25, 2018 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@paneq paneq

Labels
None yet
Projects
None yet
Milestone
v0.31
Development

Successfully merging this pull request may close these issues.
2 participants
@paneq @joelvh