search

brefphp / laravel-bridge

Package to use Laravel on AWS Lambda with Bref
https://bref.sh/docs/frameworks/laravel.html
MIT License
319 stars 63 forks source link

Store job attempt in job payload & use Laravel's built-in queue worker #31

Closed georgeboot closed 1 year ago

georgeboot commented 3 years ago

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 the attempt 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 by queue: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.

mnapoli commented 3 years ago

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.

georgeboot commented 3 years ago

but there may be bigger changes to consider and explore (e.g. integrating with more Laravel Queues features, including #14 and #22)

Any clues which ones? #14 and #22 are dealt with in this PR. I'll be happy to implement them.

Additionally, the license issue is still a problem here and I think that needs to be carefully dealt with.

Can you please point me to a resource explaining how to do copyright attribution properly? This is all I could find online.

atymic commented 3 years ago

This is awesome! Any updates?

georgeboot commented 3 years ago

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.

alexkb commented 2 years ago

I concur with @atymic, this is awesome., great work @georgeboot. Our purpose was being able to use $this->release() for Lambda messages that failed and wanted retrying later, which we can now.