active-job-style-guide
active-job-style-guide copied to clipboard
toptal
batches don't work well with ActiveJobs
https://github.com/sidekiq/sidekiq/wiki/Batches#huge-batches
Warning: this functionality is only available if you use the Sidekiq::Job API; it does not work with ActiveJob.
I know this is an old guide. Maybe mark it as obsolete if you don't have time to maintain it.
The guide was written several years ago, but the most of it still stands. I envisioned it to be a community-driven guide, so if you think that something is wrong, let’s fix it.
Can you please elaborate on what you think is off, or even better send a PR fixing the guide?
@bbatsov what are Toptal plans for this repo? I think I’ve been cut off from being able to maintain it.
There’s a section that notes that the guide creation was sponsored by Toptal. Is it fine to pass it to the community for a wider audience from being able to contribute and maintain it?
Sorry, I missed to add the part from the guide. There is a section
https://github.com/toptal/active-job-style-guide?tab=readme-ov-file#self-scheduling-jobs
Avoid using self-scheduling jobs for long-running jobs. Prefer using Sidekiq Batches to split the workload.
...
# good
class BackfillMissingDataJob < ApplicationJob
...
But this is no good. Sidekiq batches are unreliable with Sidekiq batches. Neither the Sidekiq pro batches, nor the community https://github.com/breamware/sidekiq-batch
So either native sidekiq jobs need to be used (which is obviously possible only with sidekiq or another recommendation needs to be presented. I don't know yet what would be the ideal resolution. So I can't propose a pull request. I just know from experience that current recommendation is unsafe.
Sidekiq batches are unreliable with Sidekiq batches.
with ActiveJob, got it.
Unreliable? Or don’t work as Sidekiq wiki tells?
Can you provide evidence of it not working or its reproducible unreliable behaviour?
I agree that this guideline might be outdated. But the point is not to use self-scheduling jobs. Because of “at least once”.
How does maintenance_tasks do that with ActiveJob? Do they use their job-iteration underneath? Would it be reasonable to recommend it when ActiveJob is used? Sidekiq batches still make sense when using Sidekiq directly.
This could be topic for one change.
There are a few places recommending batches. It would make sense to note that they work unreliably or don’t work with ActiveJob.
Mentioning alternatives that do work would be helpful, too.
Would you send such a PR?
Unreliable? Or don’t work as Sidekiq wiki tells?
Unreliable as they appear to work under normal condition but edge cases can easily make them break.
Can you provide evidence of it not working or its reproducible unreliable behaviour?
I don't have a reproducer but on a busy server we see some weird behavior like stale BID-### Redis entries and strangeness with the callbacks. Maybe it's fixable but not in a way that really makes things more readable and maintainable. This is the authors advise https://github.com/sidekiq/sidekiq/issues/4637, tl;dr;
I don't test batches with ActiveJob and would not recommend it.
In my book going against the recommendation of the author would hardly be a best practice.
How does maintenance_tasks do that with ActiveJob?
Do what?
Sidekiq batches still make sense when using Sidekiq directly.
Absolutely. This is not how current style guide demonstrates though.
Mentioning alternatives that do work would be helpful, too.
That's why I file an issue but not a PR. I wrote above that I don't know what makes most sense. On the one hand batches with Sidekiq can be very useful. On the other hand not everybody needs to use Sidekiq. Why have Redis as a component of your app when there are really performant ActiveJob backend implementations for postgres?
So I wouldn't put in the best practice an advise to use Sidekiq.
P.S. I'll probably replace the use of batches with self-scheduling because it makes sense and is simpler for this particular use case. Will see how it will work. But obviously this is not a good generic advice.
This guide is not about to Sidekiq or not to Sidekiq at all.
How does maintenance_tasks do that with ActiveJob? Do what?
How does it run multiple jobs.
My suggestion for you is open a PR updating those parts of the guide related to the problem that you’re certain about.
for this particular use case
I withdraw from commenting on this part because I have no awareness of what that particular use case is.
Batches break if you use Active Job’s retry_on feature. Since that is quite common, I recommend against using AJ for batches.
@pirj batches is about running multiple jobs as a kind of bundle and have callbacks on the whole batch. You can run multiple jobs without batches.
Again I say that I don't know what would be the ideal text for the guide. I know it is not about Sidekiq but it recommends sidekiq specific features presently. Which is fine. But also it is good if it includes alternatives for when people do not use sidekiq. That's why I filed an issue.
If I ever discover a generic best practice and remember, I will file a PR. So far I'm leaving it where it is.
Since we’re talking about something that some may mistakenly think will be solvable correctly with self-scheduling jobs, those are indeed not just multiple jobs, but multiple connected jobs.
ideal text for the guide
The ideal text is one that meets the criterion of leading, or guiding, the reader in the right direction when they face a problem. To write such text, you start with something, and iteratively change it to meet the above criterion.
good if it includes alternatives for when people do not use sidekiq
This is exactly what I suggest doing, see my comments above:
work unreliably or don’t work with ActiveJob. Mentioning alternatives that do work would be helpful
I have noticed how well you explain yourself in your comments, and I am pretty certain in your ability to adjust the said guidelines to help others to avoid culprits that you have had a misfortune to experience yourself.