breamware / sidekiq-batch

Sidekiq Batch Jobs Implementation
MIT License
357 stars 95 forks source link

Callback status and nested batches #26

Closed jbrady42 closed 5 years ago

jbrady42 commented 5 years ago

This is a follow up to the integration test in #25 . I'm making these separately in order to keep the tests mergable and to verify the failing case before the fix.

This PR started as an attempt to fix #13 but evolved rapidly. The problem being the status object passed to the callbacks is empty. The root of this issue is that callbacks are executed asynchronously as a job, but the redis keys holding the status are cleared when the job is pushed to Sidekiq.

To solve this, I added the concept of a "callback" batch. This is a batch that contains only the callbacks of another batch. The redis clean-up code can then be moved to the success callback of this callback batch. This means the status is available until all callbacks have completed.

In the process of testing this, I came across #11 and troubles with nested batches. It seems the logic to update a parent batch is part of the callback worker and only ever runs if there is a callback of that type registered. This breaks the propagation of events up to the batch parent.

Both of these issues fall into the same category, so the concept of "finalizing" was introduced. The finalizer is called on a batch for an event type when the batch and any callbacks for that event are complete. If there are no callbacks for an event, the finalizer is called synchronously. Otherwise it is run as a success callback on the callback batch.

Moving the event bubbling and redis cleanup to the finalizer solves both these issues and enables some complex workflows.

Notes/Issues:

Update

After seeing #24 its possible both of the above issues would be solved by ensuring success callbacks run after complete. I will attempt to combine these two approaches.

nglx commented 5 years ago

@jbrady42 would be grateful if you could find some time to fix the conflicts.

jbrady42 commented 5 years ago

Thanks for merging the tests.This was built against master before #24 which changed some of the same concepts, so I'll have to resolve the two. Fairly busy at the moment, but should have some time in the next few weeks to update this branch.

nglx commented 5 years ago

Since it's not rebased (has some conflicts) I'm merging that to a separate branch (jbrady42_pr_fixes) to continue work on that.