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

Add job-iteration integration #1217

Conversation

olivier-thatch
Copy link

This PR adds an integration for the job-iteration gem.

job-iteration needs to know when the job worker is shutting down so that it can complete the current iteration and gracefully shut down. The gem ships with integrations for Sidekiq and Resque.

My first approach was to add an integration for GoodJob to the job-iteration gem. However that proved difficult because job-iteration's test harness uses a MySQL database, so I would have to either switch everything to Postgres or set up a multiple databases. Both of these sounded more complex than necessary, so I thought I'd add the integration to GoodJob instead.

I'm opening this PR as a draft to get some early feedback:

  1. Would you be okay with adding this integration to GoodJob's codebase in the first place? It would add some maintenance burden, but the integration is trivial enough that I don't expect it to be much of an issue.
  2. The implementation in this PR probably needs a bit more work. My understanding is that GoodJob.shutdown? will only return true after GoodJob is fully shut down (i.e. when all threads have shut down). If so, that's probably too late for job-iteration's purposes. The interruption adapters for Sidekiq and Resque return true as soon as a shutdown is requested. AFAICT, there is no such hook in GoodJob at the moment. Can you confirm this is the case? If so, I'll update the PR accordingly.

@bensheldon
Copy link
Owner

I think it makes more sense for this to live in job-iteration despite their test-harness trouble. Did you propose opening a PR? I also imagine their repository is most likely where users of that library will be looking for integration support (so if this is simply something that a dev needs to monkeypatch it, I assume they'd begin by searching over there).

It's specifically this line that I have a concern because I don't want to be modifying someone's application configuration as a side-effect of installing GoodJob:

JobIteration.interruption_adapter = ....

@olivier-thatch
Copy link
Author

That's fair, though note that JobIteration.interruption_adapter is not a user-visible configuration value. It's set automatically depending on which job worker process is used, so it doesn't matter if it's set by job-iteration's own code or by the ActiveJob backend's, as long as it's set correctly.

I'll reach out to job-iteration's maintainers to see what they think.

Can you let me know your thoughts on my 2nd question?

@Mangara
Copy link

Mangara commented Jan 18, 2024

It's specifically this line that I have a concern because I don't want to be modifying someone's application configuration as a side-effect of installing GoodJob.

We're working to change the way interruption adapters are configured in job-iteration to something that is based on the queue adapter name. In the new version it would look more like

JobIteration.register_interruption_adapter(:good_job, ...)

That interruption adapter would then automatically be used, but only for jobs that use GoodJob as their queue adapter.

Point 2 is important - the interruption adapter should return true as soon as the graceful termination window starts.

@olivier-thatch
Copy link
Author

Thanks @Mangara, the new version looks preferable since I think it's technically possible to use different backends on a per-job basis (though that's probably a very uncommon setup).

Regarding point 2, I'm happy to submit a PR to GoodJob to make it possible to be notified as soon as a shutdown is requested, if @bensheldon is okay with the idea.

@bensheldon
Copy link
Owner

Oops, sorry for overlooking the 2nd question.

You're correct there isn't a GoodJob.shutting_down?. It's not possible to have a global status for that because it's possible to have different job executors (GoodJob::Scheduler) in different intentional states (some running, some shutdown).

It looks like the the InterruptionAdapter#call is invoked within the context of a job being executed. I think the way to handle will take a little bit of work, but shouldn't be too difficult (though I am likely to wonder what the best public interface is to expose).

GoodJob assigns some thread-local variable when executing jobs (GoodJob::CurrentThread). When a GoodJob::Scheduler begins executing the job, we could store a reference to the scheduler that then could be checked like `GoodJob::CurrentThread.scheduler.shutting_down? (That isn't implemented but could be easily added).

Caveat: Jobs that are run :inline, or via GoodJob.perform_inline are not run within a scheduler so I imagine in that case it would just run to finish.

@olivier-thatch
Copy link
Author

olivier-thatch commented Jan 24, 2024

Thanks for the reply @bensheldon.

job-iteration would need to check if the GoodJob scheduler is shutting_down? || shutdown?. But shutting_down? is the same thing as !(running? || shutdown?), so this boils down to !running?.

I'm happy to add a GoodJob::Scheduler#shutting_down? helper if you think that's userful, but job-iteration should probably use !running? since that would make it compatible with a broader range of GoodJob versions. Does this sound reasonable to you?

EDIT: I missed that GoodJob::CurrentThread.scheduler does not exist today, so compatibility would be limited to new GoodJob versions anyway 😞

@olivier-thatch
Copy link
Author

We ended up using the scheduler_shutdown_start.good_job ActiveSupport notification. It doesn't let us know which GoodJob process is shutting down specifically, but it's fine for our purposes. I posted the full code for our integration here: Shopify/job-iteration#453 (comment).

Closing this PR as I am not planning on working on this any further at this time.

@olivier-thatch olivier-thatch deleted the olivier-job-iteration-integration branch February 8, 2024 22:38
@bensheldon
Copy link
Owner

Closing the loop on this, I added a methods for interrogating graceful shutdown: #1253

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

Successfully merging this pull request may close these issues.

3 participants