Infer interruption handler from a job's queue adapter

[bdewater]

... and allow Iteration to be used with multiple job backends simultaneously. Closes #96, it's based on this PR but it looks abandoned.

The Fallback adapter mimics previous behaviour if self.interruption_adapter = -> { false } was not overridden for Resque or Sidekiq, but to me it seems more correct to raise an error instead of no interruption adapter is found. If the maintainers are open to a breaking change I can make it so.

Removed test_mark_job_worker_as_interrupted since it was testing stubs.

[Mangara]

Can we keep JobIteration.interruption_adapter around for queue adapters other than Sidekiq or Resque? Or should we give them a way to hook into the discovery? One tricky bit there is that queue_adapter_name can return class if you do something like config.active_job.queue_adapter = OtherQueueAdapter.

If the maintainers are open to a breaking change I can make it so.

Removing a configuration option is already a breaking change 🙂 Even without that, merged unreleased changes include dropping support for older Ruby versions and such, so we're already looking at a major release I think. I'm fine wth raising if no interruption configuration is found.

[bdewater]

Can we keep JobIteration.interruption_adapter around for queue adapters other than Sidekiq or Resque? Or should we give them a way to hook into the discovery? One tricky bit there is that queue_adapter_name can return class if you do something like config.active_job.queue_adapter = OtherQueueAdapter.

TIL (updated) about that way of assigning a queue adapter but it seems queue_adapter = OtherQueueAdapter would make queue_adapter_name return "other_queue"?

I'm currently looking at an adapter for https://github.com/bensheldon/good_job, let me experiment with that this week and see how it goes. I don't think keeping JobIteration.interruption_adapter around makes much sense, I can't imagine where you'd want a Resque interruption adapter for a Sidekiq-backed job? Perhaps I'm missing something here.

Removing a configuration option is already a breaking change 🙂

Ha, fair point :D

[Mangara]

I guess it's a bug / oversight in that Rails code:

queue_adapter = OtherQueueAdapter.new  ->  queue_adapter_name == "other_queue"
queue_adapter = OtherQueueAdapter ->  queue_adapter_name == "class"

I can't imagine where you'd want a Resque interruption adapter for a Sidekiq-backed job? Perhaps I'm missing something here.

There should be some way for queue adapters that job-iteration does not know about to provide an appropriate interruption adapter. At Shopify we use an internal fork of Resque that is no longer compatible with the standard interruption adapter for Resque, so we need a way to override it.

[bdewater]

There should be some way for queue adapters that job-iteration does not know about to provide an appropriate interruption adapter. At Shopify we use an internal fork of Resque that is no longer compatible with the standard interruption adapter for Resque, so we need a way to override it.

IIRC it had an owl-related name 😉 I was aiming for defining a module/class under the JobIteration::Integrations namespace (following Active Job queue_adapter_name convention) to be enough, similarly to how GoodJob registers itself with AJ. But I'm assuming you're running into that bug you mentioned so I added a way to override the loading logic while still ensuring it is job-based and not necessarily a global override.

[Mangara]

Can we swap these to class << self blocks? We don't need the methods to be defined at the instance level, right? (This goes for all of the extend self here, I think.)

[Mangara]

I was aiming for defining a module/class under the JobIteration::Integrations namespace

Interesting. Recommending that users monkey patch your library feels off to me, there must be better ways to provide that configuration.

How do you feel about a lookup table from queue_adapter_name to interruption adapter that users can add to?

Something like

JobIteration.register_interruption_adapter("fancy_queue", ->() { FancyQueue.should_interrupt? })

That way, queue adapters that trigger the Rails bug can just register for "class" and we don't have to work around it here. (I fixed the bug, btw 🙂, although that only helps for Rails 7.1 and on)

I think I prefer that over overriding the entire loader.

[bdewater]

Interesting. Recommending that users monkey patch your library feels off to me, there must be better ways to provide that configuration. How do you feel about a lookup table from queue_adapter_name to interruption adapter that users can add to?

Maybe I was pattern matching too much on Active Job/GoodJob, your suggestion makes sense to me.

That way, queue adapters that trigger the Rails bug can just register for "class" and we don't have to work around it here. (I fixed the bug, btw 🙂, although that only helps for Rails 7.1 and on)

Awesome 🙌 it would be useful to backport to 7.0 IMO, for those of us not living on Rails main 😛 I don't think it returning "module" is useful/expected.

[plasticine]

Thanks for picking this up @bdewater 👏 priorities changed a bit and I never got back to #96, so thanks for pushing it forward!

[bdewater]

Gentle nudge @Mangara :)

[Mangara]

Sorry for the long wait. We're actually working on a release this week. We'll do a point release first with all the non-breaking changes. Once that's out, we can merge this and prep for 2.0.

[Mangara]

I like where this is at now! Just a few comments.

[Mangara]

Thanks for all your hard work on this, and apologies for the long delay!

[lavoiesl]

This should register a dummy handler, not raise.
Otherwise, this results in an exception for any queue adapter that isn't recognized

[bdewater]

This change was by design, see the discussion above regarding a Fallback adapter I originally included but then removed.

The problem with the old behaviour is that (depending on the queue adapter) it can lead to lost jobs if the process cannot re-enqueue jobs and shutdown gracefully, since it would never interrupt the job. This is no longer silently allowed to happen. If you need this to work as it used to, you can register your own interruption adapter with: JobIteration::Integrations.register("queue_adapter_name", -> { false })

It is a breaking change that should be called out explicitly in the changelog, I opened for that: #425

[lavoiesl]

But this breaks the users (our team uses qless (yes, I know, it's unmaintained, we're moving away from it))

How about this:

• Add a graceful fallback, preserving the current behaviour, but with a deprecated notice
• Release a patch version on the current minor version
• Plan to remove the graceful fallback in the next major version

[bdewater]

This PR will be part of a new major release to my understanding, because there are other breaking changes: if you had written your own interruption adapter for active_qless you can't set it via JobIteration.interruption_adapter anymore. Makes sense to break related things together :)

Is there a reason you can't register your own adapter like in my previous message if you need to be on latest main? Alternatively, cut a 1-0-stable branch starting from 7e6c385 and cherry-pick non-breaking changes from main and cut a new point release if for some reason you absolutely have to use Iteration from Rubygems?

[lavoiesl]

That's pretty much what I ended up doing, yes, but in general, I think it'd be good to keep a non-breaking 1.x.

Merging to main without already having a plan to make a new major release means that the releaser will have a tougher challenge on their hands, having to disentangle which changes are breaking and which aren't.

Perhaps we're already at a point where we should be starting a 1.x branch?

I assume that after we release 2.x, we'll likely want to backport some fixes to 1.x

[Mangara]

I assume that after we release 2.x, we'll likely want to backport some fixes to 1.x

Because the breaking change is fairly minimal (primarily replacing JobIteration.interruption_adapter with JobIteration::Integrations.register), I'm not sure it's worth the effort to maintain a separate 1.x branch and backport fixes to there. I was planning to release 2.0 pretty soon, without cutting another minor version in-between.

[lavoiesl]

Alright then 👍🏼