-
Notifications
You must be signed in to change notification settings - Fork 64
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
Store job attempt in job payload & use Laravel's built-in queue worker #31
Conversation
* @author Taylor Otwell <[email protected]> | ||
* @copyright Copyright (c) 2019, Taylor Otwell |
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.
@mnapoli how do you want me to give attribution in this code? phpdoc references are not allowed.
Hey @georgeboot, thanks for iterating on this following the first PR. At the moment unfortunately I don't have enough time to spend a few days on this (I wish I could). I think the general approach makes sense, but there may be bigger changes to consider and explore (e.g. integrating with more Laravel Queues features, including #14 and #22). The target isn't 100% defined yet I believe. Additionally, the license issue is still a problem here and I think that needs to be carefully dealt with. Given the time it requires, I think there are the following scenarios of what could happen:
In the meantime unfortunately, I don't see this topic moving forward much. |
Any clues which ones? #14 and #22 are dealt with in this PR. I'll be happy to implement them.
Can you please point me to a resource explaining how to do copyright attribution properly? This is all I could find online. |
This is awesome! Any updates? |
For now, you can use my fork. I've been using it in production without issues for couple of months now, so it's stable I guess. I have some more things I usually add to my projects to make Bred behave more "Laravel-like" (if that is even a word). I will probably create a separate package that will include all these things. People can than choose between bref/laravel-bridge and my package, depending on their needs and preferences. |
I concur with @atymic, this is awesome., great work @georgeboot. Our purpose was being able to use |
* Report job failures to failed jobs provider * sort imports Co-authored-by: Till Krüss <[email protected]>
Fixes #14
Fixes #22
Change 1: Store job attempt in job payload
ApproximateReceiveCount
should not be used when processing SQS jobs in Lambda, as it is very unreliable. See https://link.medium.com/uUkT2W9bTbb for more info.This PR adds functionality to store the current attempt on the job payload, so we don't have to depend on
ApproximateReceiveCount
. This effectively means that theattempt
value should be correctly set on every try. Because we can't update SQS messages, we always have to re-create a message, rather than releasing it back onto the queue.This change will make
$this->job->attempts()
and descendants of it, ($maxExceptions
,$tries
), usable.Change 2: Refactor workers
This PR also changes the queue worker to use the frameworks built-in worker, in favour of the purpose built worker from this package. Now that we don't have to deal with the
ApproximateReceiveCount
, there is not need to use our own worker class, and we should rather use the default one provided by Laravel. This also means that jobs are now processed by the same system used byqueue:work
and Laravel Horizon, limiting the changes between platforms.Note that Laravel's queue worker expects a maintenance mode status. As this feature is not implemented in this package, the value is hardcoded to
false
. This effectively means that the queue worker will simple ignore any maintenance mode set. This is in line with earlier versions of this package, that also ignored the maintenance mode.The code is 'backwards compatible' in the sense that the example
worker.php
remains identical. If you did not change that, all should continue working as usual.The code is heavily inspired (and sometimes copied) from https://github.com/laravel/vapor-core/. Attributions were giving where due.