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] Job Batching #32830

Merged
merged 45 commits into from May 18, 2020
Merged

[8.x] Job Batching #32830

merged 45 commits into from May 18, 2020

Conversation

taylorotwell
Copy link
Member

This PR implements job batching:

Dispatching Batches

image

The Bus::batch method returns an Illuminate\Bus\Batch instance which may be inspected to gather the ID and other information about the batch. The batch instance is also JSON serializable so it may be returned directly from a route:

image

When dispatching batches, you may attach various callbacks to the batch using then, success, and catch.

In addition, you may use the allowFailures method to indicate that a batch should not be automatically cancelled when a job within the batch fails. If this method is not called, the batch will automatically be cancelled when a job within the batch fails.

Querying Batches

Batch meta-data storage is backend agnostic behind a BatchRepository interface. I have implemented a database implementation. You may retrieve an existing batch using the Bus::findBatch method, which either returns an Illuminate\Bus\Batch instance of null:

image

The Batch instance has various helper methods on it, such as finished, cancelled, cancel, etc.

Batchable Jobs

Batchable jobs should use a new Batchable trait:

image

At the beginning of a batched job's handle method, it may detect if the batch has been cancelled and act accordingly:

image

Adding Additional Jobs To A Batch

Within a batched job, you may add additional jobs to the batch using the add method on the batch instance. You should never add jobs to an existing batch outside of a batched job:

image

Failed Job Handling

The UUIDs of the failed jobs within a batch are stored with the batch meta data. A new queue:retry-batch {batch-uuid} command has been added to easily retry all of the failed jobs for a given batch. Note that you can only retry jobs for a batch that allows failures.

image

Supporting this feature required the addition of a new failed job backend implementation for relational databases that uses the job's UUID instead of an auto-incrementing primary key to reference jobs. I decided to make this a new implementation (which will be the default in Laravel 8.x) since it would be easier to allow users to continue using the existing version until they actually need batches.

Testing

You may test the dispatching of batches using Bus::fake and Bus::assertBatched:

image

@GrahamCampbell GrahamCampbell changed the title Job Batching [8.x] Job Batching May 15, 2020
@taylorotwell
Copy link
Member Author

@SjorsO I'll just add an alias so either will work.

array $failedJobIds,
array $options,
CarbonImmutable $createdAt,
?CarbonImmutable $cancelledAt = null,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Usually laravel does not use the questionmark and implements nullable types with the default value being null. Not both.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is legacy PHP behavior which IMO shouldn't be used anymore. The new database factory code (which was also written by Taylor) also uses the question mark.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://wiki.php.net/rfc/typed_properties_v2

Typed properties cannot have a null default value, unless the type is explicitly nullable (?Type). This is in contrast to parameter types, where a null default value automatically implies a nullable type. We consider this to be a legacy behavior, which we do not wish to support for newly introduced syntax.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Many people still use the style without the question mark. Including me.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(and the rest of the laravel/framework source code)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If laravel wants to change its code style to use the question mark, StyleCI can enforce this...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It can also enforce non-usage if we want to enable that rule on the laravel preset.

@rasmuscnielsen
Copy link
Contributor

Looks awesome 🎉

Was wondering what happens when a batch gets cancelled?
Would expect the remaining pending jobs to get cancelled, but from the example it seems that this has to be implemented manually in the individual jobs' handle() methods?

@SjorsO
Copy link
Contributor

SjorsO commented May 16, 2020

@SjorsO I'll just add an alias so either will work.

I think consistent American English spelling throughout the framework would be a better idea. That way teams don't have to bikeshed about canceled/cancelled, color/colour, gray/grey, etc

The bikeshedding I'm doing right now could also have been preventing by being consistent 😄

}

/**
* Get the total number of jobs that have been processed by the batch thus far.
Copy link

@ghost ghost May 16, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where is the return type?

Should be;
@return int

* Retrieve information about an existing batch.
*
* @param string $batchId
* @return \Illuminate\Bus\Batch
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be;

@return \Illuminate\Bus\Batch|null

@themsaid
Copy link
Member

@rasmuscnielsen that's right. Before each job you need to check if the batch is still active before you attempt to run the code.

@anouarabdsslm
Copy link
Contributor

anouarabdsslm commented May 16, 2020

Will be waiting for this to get merged. thanks guys for this cool feature

@taylorotwell
Copy link
Member Author

@rasmuscnielsen that is correct. that is somewhat modeled after Sidekiq Pro's approach in Ruby. It is the most flexible approach. We could later add helpers on the batch itself to make this more automatic...

Bus::batch(...)->failCancelled()

Or something to that effect...

@ahmedash95
Copy link
Contributor

Nice feature, Is there any way to get the failed jobs instead of the number of failers?

<?php

->catch(function(Batch $batch, $e){
       $batch->failedJobs->each(function($job){
             info(get_class($job) .' faild with exception: '. $e->getMessage());
      })
})

@themsaid
Copy link
Member

@ahmedash95 there's a $failedJobIdspublic property on the Batch instance that you can use.

@ahmedash95
Copy link
Contributor

ahmedash95 commented May 18, 2020

@ahmedash95 there's a $failedJobIdspublic property on the Batch instance that you can use.

Perfect 👌🙏

@taylorotwell
Copy link
Member Author

Note that you would only be able to get the full list of failedJobIds from the then callback though.

@taylorotwell taylorotwell merged commit 63730ec into master May 18, 2020
@taylorotwell taylorotwell deleted the batches branch May 18, 2020 19:09
@taylorotwell
Copy link
Member Author

Noting that since this was merged... success has been renamed to then. What was then is now finally. This better matches JS callback terminology.

Copy link
Contributor

@mfn mfn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This probably wasn't caught as there being no tests for the fake.

I got this when I ran parts of laravel test suites with pcov code coverage, which I guess triggered loading of this class and then failed.

* @param string $batchId
* @return int|null
*/
public function decrementPendingJobs(string $batchId)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is BatchRepositoryFake implements BatchRepository but the method on the interface is public function decrementPendingJobs(string $batchId, string $jobId);, as such this doesn't work due to:

PHP Fatal error: Declaration of Illuminate\Support\Testing\Fakes\BatchRepositoryFake::decrementPendingJobs(string $batchId) must be compatible with Illuminate\Bus\BatchRepository::decrementPendingJobs(string $batchId, string $jobId) in …/src/Illuminate/Support/Testing/Fakes/BatchRepositoryFake.php on line …

* @param string $batchId
* @return int|null
*/
public function incrementFailedJobs(string $batchId)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar error as above, interface method is public function incrementFailedJobs(string $batchId, string $jobId);

PHP Fatal error: Declaration of Illuminate\Support\Testing\Fakes\BatchRepositoryFake::incrementFailedJobs(string $batchId) must be compatible with Illuminate\Bus\BatchRepository::incrementFailedJobs(string $batchId, string $jobId) in …src/Illuminate/Support/Testing/Fakes/BatchRepositoryFake.php on line …

LukeTowers pushed a commit to wintercms/winter that referenced this pull request Nov 15, 2023
bennothommo pushed a commit to wintercms/wn-system-module that referenced this pull request Nov 15, 2023
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