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

[8.x] Allow for chains to be added to batches #34612

Merged
merged 6 commits into from Oct 7, 2020
Merged

[8.x] Allow for chains to be added to batches #34612

merged 6 commits into from Oct 7, 2020

Conversation

Orrison
Copy link
Contributor

@Orrison Orrison commented Oct 1, 2020

This adds functionality to batching that allows you to pass in a sub array of job classes that will create a job chain included in the batch.

The first item in the array will be the "chain head" and be the job that the other jobs are chained onto.

It would be used as so:

$batch = Bus::batch([
            new Test1Job(),
            [
                new Test2Job(),
                new Test3Job(),
                new Test4Job(),
            ]
        ])->then(function (Batch $batch) {
            Log::info('Test Complete!');
        })->name('Test')->dispatch();

Each job is given a batchId just as they normally would and the then only fires once all are completed.

In the above example Test3Job and Test4Job are chained onto Test2Job.

Test1Job executes on it's own NOT in the chain with the other 3 just as batching works now.

This benefits users as batching, as it works now ,provides amazing support for doing a bunch of things and doing something when they are completed.
But sometimes some of those things have to happen in a particular order BUT others can happen at any time.
This makes that easier without having to add the job to the batch from within another job. Making it easier to track, at a high level, what is happening and in what, if any, sequence.

Orrison added 2 commits Oct 1, 2020
Signed-off-by: Kevin Ullyott <ullyott.kevin@gmail.com>
Signed-off-by: Kevin Ullyott <ullyott.kevin@gmail.com>
@driesvints driesvints changed the title Feature: Allow for chains to be added to batches [8.x] Allow for chains to be added to batches Oct 1, 2020
Signed-off-by: Kevin Ullyott <ullyott.kevin@gmail.com>
@Orrison
Copy link
Contributor Author

@Orrison Orrison commented Oct 1, 2020

Hmm did I mess something up that is causing so many of these tests to fail? Something to do with redis extension not being installed?

@Orrison
Copy link
Contributor Author

@Orrison Orrison commented Oct 1, 2020

Oh never mind I see there is an issue with serialization of the anonymous function in the PHP73 tests. Looking into that

Orrison added 3 commits Oct 1, 2020
Signed-off-by: Kevin Ullyott <ullyott.kevin@gmail.com>
Signed-off-by: Kevin Ullyott <ullyott.kevin@gmail.com>
Signed-off-by: Kevin Ullyott <ullyott.kevin@gmail.com>
@themsaid
Copy link
Member

@themsaid themsaid commented Oct 2, 2020

If you do this. Supporting this will be requested:

$batch = Bus::chain([
    new Test1Job(),
    [
        new Test2Job(),
        new Test3Job(),
        new Test4Job(),
    ],
    new Test5Job(),
])->then(function (Batch $batch) {
    Log::info('Test Complete!');
})->dispatch();

Where TestJobs 2,3,4 are within a batch.

There are also more complex workflows that may be required since we added support for those, like nested batches inside chains inside batches etc...

IMO we should keep batches and chains 2 simple separate features and people can easily combine them to achieve the desired behaviour. Let's see what Taylor thinks.

@Orrison
Copy link
Contributor Author

@Orrison Orrison commented Oct 2, 2020

That makes a lot of sense @themsaid. I did see your post on doing the opposite, firing off a batch from within a chain, that was a very simple way of combining the power of both.

I totally understand the slippery slope this could create of people requesting nesting deeper and deeper with this nested array concept.
But I do still see a use case for including a job chain in a batch and having the batch track each job in that chain. So that then completion can be fired once all are completed.

The format of accomplishing that I am not adhered too. If the nested arrays is too much I could look into including the chain in a more native way, perhaps getting it to work with a PendingChain? Something similar to:

$batch = Bus::chain([
    new Test1Job(),
    Bus::chain([
        new ProcessPodcast,
        new OptimizePodcast,
        new ReleasePodcast,
    ]),
    new Test5Job(),
])->then(function (Batch $batch) {
    Log::info('Test Complete!');
})->dispatch();

@Orrison
Copy link
Contributor Author

@Orrison Orrison commented Oct 6, 2020

@taylorotwell with the other PR closed. What are your thoughts on this implementation of the possible feature?

@taylorotwell
Copy link
Member

@taylorotwell taylorotwell commented Oct 6, 2020

I think it's generally fine. IMO just because we accept this feature does not mean we are required to accept any other complex feature. We aren't required to accept any features at all. 😅

We have had a couple of requests for this and I can see the use case so I think it's generally fine.

Question: does this work if I add jobs to the batch from within a batched job?

$this->batch()->add([
    new Job,
    [
        // Chained jobs...
    ],
]);

@Orrison
Copy link
Contributor Author

@Orrison Orrison commented Oct 6, 2020

@taylorotwell yes it does! I have tested it locally at it works. It increments the jobs total on the batch and only fires the completion when all are finished.

I can try and write a test for it as well if you prefer?

@taylorotwell taylorotwell merged commit eb4f307 into laravel:8.x Oct 7, 2020
7 checks passed
@taylorotwell
Copy link
Member

@taylorotwell taylorotwell commented Oct 7, 2020

Thanks.

@Orrison
Copy link
Contributor Author

@Orrison Orrison commented Oct 7, 2020

@taylorotwell with this being included in next weeks release is someone updating the Queues documentation to include this? I could put together a PR for that if no one else is currently?

@driesvints
Copy link
Member

@driesvints driesvints commented Oct 8, 2020

@Orrison yeah feel free to send in a PR for that. Much appreciated, thanks 👍

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

Successfully merging this pull request may close these issues.

None yet

4 participants