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

[5.2] Add failed job ID when dispatching a JobFailed event #13920

Merged
merged 3 commits into from
Jun 9, 2016

Conversation

kevinsimard
Copy link
Contributor

@kevinsimard kevinsimard commented Jun 8, 2016

We wrote a piece of code in our Laravel application which allows us to add exception information in a separate table when a job fails. The purpose of this is was to be able to have more knowledge on what happened to a specific failed job before rerunning it with php artisan queue:retry <id>. The new table has columns like the exception message, backtrace, data and some user information.

The problem that we were facing was that the ID from the failed_job table wasn't recoverable once the failer instance was inserting a record in the database because that function wasn't returning anything (void).

This could allow people to check and play with the failed job ID when a JobFailed event is dispatched in the application. I also added a default value of NULL to make sure that this isn't breaking the previous event class definition.

* Create a new event instance.
*
* @param string $connectionName
* @param \Illuminate\Contracts\Queue\Job $job
* @param array $data
* @param int $failedId
Copy link
Member

@GrahamCampbell GrahamCampbell Jun 8, 2016

Choose a reason for hiding this comment

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

int|null

@kevinsimard
Copy link
Contributor Author

@GrahamCampbell Fixed

@taylorotwell
Copy link
Member

Not sure it's really necessary to have the value be optional. There is now way to feasible extend this event from the core I don't think.

@taylorotwell taylorotwell merged commit 5757f48 into laravel:5.2 Jun 9, 2016
@taylorotwell
Copy link
Member

So now that you have the ID how do you use that to update the table with the exception information? Or how do you link the two?

@kevinsimard
Copy link
Contributor Author

kevinsimard commented Jun 9, 2016

@taylorotwell I created a second table with the following columns

id
# failed_job ID
failed_job_id
# exception name
name
# exception message
message
# exception backtrace
backtrace
# exception data
data
# creation date
created_at

I also added a foreign key between failed_job_id and failed_job.id with a onDelete('cascade') which will remove the row from my new table automatically whenever a job is retried with php artisan queue:retry <id>.

The reason why this is so important for us is because we have a lot of failed jobs and sometimes it's kind of hard to know what really happened to that job so we first started by adding the backtrace to the payload (column in the failed_job table) but we didn't wanted to add all the other columns in there so we thought adding a new table would be better and easier to filter if we ever want to retry all jobs for a certain type of exceptions.

@kevinsimard
Copy link
Contributor Author

Do you want me to create a new PR to remove the optional value?

@GrahamCampbell GrahamCampbell changed the title Add failed job ID when dispatching a JobFailed event [5.2] Add failed job ID when dispatching a JobFailed event Jun 9, 2016
@taylorotwell
Copy link
Member

Nah it’s OK I left it because SyncJobs have no ID to pass.

Regards,
Taylor

On Jun 8, 2016, at 9:33 PM, Kevin Simard [email protected] wrote:

Do you want me to create a new PR to remove the optional value?


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub #13920 (comment), or mute the thread https://github.com/notifications/unsubscribe/AAcRfrwsNtQV_ZajCPNUgmNWZTC-rtNsks5qJ3thgaJpZM4IxSwv.

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.

3 participants