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

Switch to Laravels own queue worker #13

Merged
merged 16 commits into from
Nov 11, 2022

Conversation

georgeboot
Copy link
Contributor

@georgeboot georgeboot commented Nov 1, 2022

This PR adds a more native queue handler. It tries to utilise as many internal Laravel components as possible, to stay as close to the other queue implementations as possible.

It's worth noticing that due to the way Lambda timeouts and SQS's integration with Lambda works, it's probably best to run with a batchSize of 1. If you want a larger batch size, make sure your jobs are designed in such a way that it won't break things if they are run more than once.

This package will add a timeout value (calculated for each job by subtracting a safety margin from the remaining invocation time) that will trigger a job timeout exception if the job is configured to do so.

Configuring a job to raise a failed exception on timeout can be done by adding the following to the job class:

public $failOnTimeout = true;

// or

public function shouldFailOnTimeout()
{
    return true;
}

When is this is set, the handler will catch the timeout, flag the job as failed and push it back onto the queue with it's attempts incremented with 1.

If the above is not set, the worker will not catch the timeout, and after SQS's visibility timeout expires, the job will be retried with the attempts NOT incremented.


Closes #6

Tested by hand using a test project deployed using sls.

@georgeboot georgeboot marked this pull request as ready for review November 1, 2022 15:46
use Illuminate\Queue\Events\JobProcessed;
use Illuminate\Queue\Events\JobProcessing;
use Illuminate\Queue\Events\JobExceptionOccurred;
use CacheWerk\BrefLaravelBridge\MaintenanceMode;

class QueueHandler extends SqsHandler
Copy link
Member

Choose a reason for hiding this comment

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

Vapor uses the VAPOR_QUEUE_DATABASE_SESSION_PERSIST option to keep database connections open between SQS events, should we add this as well?

Copy link
Member

Choose a reason for hiding this comment

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

Choose a reason for hiding this comment

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

@tillkruss Without a doubt!

Copy link
Member

Choose a reason for hiding this comment

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

@JackEllis This looks good to me now. Any feedback?

@georgeboot pointed out in Slack:

Currently there are no database disconnects, which means connections always stay open between invocations. AFAICT this is the case for web, artisan and queue. Only the octane client optionally disconnects.

@georgeboot georgeboot requested a review from tillkruss November 9, 2022 13:47
@tillkruss
Copy link
Member

If the above is not set, the worker will not catch the timeout, and after SQS's visibility timeout expires, the job will be retried with the attempts NOT incremented.

How is this not the framework default?! 🤷‍♂️

@georgeboot
Copy link
Contributor Author

Jup, good question. We could reverse the behaviour and enable it by default? Would be a better 'out of the box' experience.

@tillkruss
Copy link
Member

Jup, good question. We could reverse the behaviour and enable it by default? Would be a better 'out of the box' experience.

Yes please.

@georgeboot
Copy link
Contributor Author

Yes please.

done

@tillkruss tillkruss requested a review from JackEllis November 9, 2022 18:21
@tillkruss tillkruss merged commit 46ab5d2 into cachewerk:main Nov 11, 2022
@georgeboot georgeboot deleted the feature/add-laravel-queue-worker branch November 11, 2022 16:55
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.

Better queue handler
3 participants