Infer interruption handler from job #96
Conversation
plasticine
force-pushed
the
plasticine/infer-interruption-handler
branch
from
4ffe632
to
47adc55
Compare
…self Instead of automatically attempting to load the interruption adapter implicitly this patch changes this to be discovered for a given worker based on the job itself based on its class adapter. I believe this will work OK, and should allow a bit more flexibility around using JobIteration. This patch also contains a bunch of renames, broadly from `job-iteration` -> `job_iteration`. This might be a bit contentious, but the current module naming breaks autoloading a bit, which is not ideal, and I’d like to leverage it for loading integrations.
plasticine
force-pushed
the
plasticine/infer-interruption-handler
branch
from
47adc55
to
fc68076
Compare
|
Hey all 👋 — rebased this again, but keen to hear feedback on whether this patch is desirable or not. Very keen to try and get |
|
Hey @sambostock @adrianna-chang-shopify 👋 Sorry for a ping on this directly, but I’d be really interested in some feedback here if either of you has a moment (or can redirect this in a better direction). |
|
Hey @plasticine ! I'll nudge @GustavoCaso and @Mangara from our Jobs team -- they probably have more context on potential consequences to inferring the integration from the queue adapter inside the iteration code, instead of setting it up front based on what integration can be loaded. It's a little hard to parse the diff because of the rename -- maybe that could get split into a separate commit up front, with the changes to the integration code committed separately? |
I'd prefer a separate PR even. |
| extend ActiveSupport::Autoload | ||
|
|
||
| autoload :SidekiqIntegration | ||
| autoload :ResqueIntegration |
There was a problem hiding this comment.
For what it's worth, this could just use Ruby's built-in autoloading, which wouldn't require the rename.
| extend ActiveSupport::Autoload | |
| autoload :SidekiqIntegration | |
| autoload :ResqueIntegration | |
| autoload :SidekiqIntegration, "#{__dir__}/integrations/sidekiq_integration.rb" | |
| autoload :ResqueIntegration, "#{__dir__}/integrations/resque_integration.rb" |
From there, a separate PR could propose the rename and use of ActiveSupport::Autoload, though I'm not sure there is much benefit, since the autoloading is so limited.
There was a problem hiding this comment.
Argh — I didn’t even consider this, thanks
|
Hey all, thanks very much for the feedback, and apologies again for the messiness of this patch — it got a bit out of hand as I was hacking around. 🙃 How does the core idea of inferring the integration strike y’all?—If that’s palatable then I’ll rework this patch into something a little less unwieldy without the renaming. |
It makes sense to me. |
|
Makes a lot of sense to me too - since Rails supports specifying the queue adapter on a per-job basis, it seems unnecessarily inflexible to force Job Iteration users to stick to a single queuing backend for their whole application. |
This was a bit of a quick attempt to fix #90
Firstly I wanted to apologise for the size of this patch, and how disruptive the renaming here is—understand this might be a non-starter for various reasons, but I was keen to use autoloading and the current naming scheme breaks this AFAIK.
Happy to try and rework and open to suggestions on how to proceed here if this patch is otherwise of interest. 🙃
If this patch gets accepted I’ll be very keen to follow up with further patches for #88 & #89.