-
-
Notifications
You must be signed in to change notification settings - Fork 200
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
Implement GoodJob::Batch
#712
Conversation
861a7f2
to
b8965ad
Compare
README.md
Outdated
end | ||
``` | ||
|
||
- Jobs can be added to an existing batch. Jobs in a batch are enqueued and performed immediately, though the final callback job will not be enqueued until `GoodJob::Batch#enqueue` is called. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I find this instruction unclear. To me, it looks like the code below contains a race condition. What happens if all the jobs finishes before enqueue
is called?.
On another note: What happens if you call enqueue
twice on the same batch?
What happens if you call enqueue
on a batch without any jobs?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great callout! I will cover that in the documentation. Briefly:
- the callback job won't be enabled until
#enqueue
is called on the batch. - calling
#enqueue
on a batch without any jobs will immediately enqueue the callback job #enqueue
can be called multiple times, which will re-enable the callback job to be enqueued if it had previously been triggered because all jobs had finished.#add
will add jobs to the batch without triggering an enqueue
Do you think this kind of example makes sense for creating a multistage batch?
class MyBatchJob < ApplicationJob
def perform(batch, options = {})
if batch.properties[:stage] == 1
batch.properties[:stage] = 2
batch.enqueue do
10.times { OtherJob.perform_later}
end
elsif batch.properties[:stage] == 2
batch.properties[:stage] = 3
batch.enqueue do
OtherJob.perform_later
end
else
# all done
end
end
end
GoodJob::Batch.enqueue(MyBatchJob, stage: 1)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have not have time yet to go over the code in detail, but I've written some questions that popped up when reading the documentation.
All in all, this looks very, very good to me. Nice API, seems like a pretty clean implementation, and an absolute KILLER feature.
I think the documentation would be even better with an example of a complex workflow (using batches to run both serial and parallel jobs in succession.) E.g, show how one would implement this workflow:
1ad77ff
to
cf22c56
Compare
Implementation of GoodJob::Batch Clean up thread globals as much as possible Remove `jitter:` which is not broadly Rails compatible and fix renamed kwarg Fix migration generator spec Fix Rspec example_app directory Update GoodJob::Batches Dashboard; rename finished/discarded Add description; add static helper in batches extension, fix migration Add batch#show screen Add batch cleanup Fix duplicate description key Fix linting Fix cleanup_preserved_jobs query Allow compatibility without database migration Ensure Batch#properties is updated on save Add documentation
cf22c56
to
dd3cdd8
Compare
… minimize public interface
Connects to #417, #691.
params
toproperties