Add failed jobs support #103

sheldonreiff · 2022-10-19T17:58:34Z

This adds support for storing and manually requeuing failed jobs (defined as jobs that have exceeded maxAttempts). This would lay some groundwork for #11.

This is a somewhat significant set of changes so I'm very open to any feedback.

Many thanks to @amayer5125 for his suggestions and review.

markstory

Nice work 👏 Thank you for putting this together. I left a few comments on naming conventions and documentation improvements.

markstory · 2022-10-24T02:37:34Z

config/Migrations/20221007202459_CreateFailedJobs.php

+     */
+    public function change()
+    {
+        $table = $this->table('failed_jobs');


Suggested change

$table = $this->table('failed_jobs');

$table = $this->table('queue_failed_jobs');

Namespacing the table seems prudent when you want to use a name this generic.

markstory · 2022-10-24T02:38:07Z

docs/en/index.rst

@@ -55,13 +55,23 @@ The following configuration should be present in the config array of your **conf

            // The amount of time in milliseconds to sleep if no jobs are currently available. default: 10000
            'receiveTimeout' => 10000,
+
+            // Whether to store failed jobs in the failed_jobs table. default: false


Suggested change

// Whether to store failed jobs in the failed_jobs table. default: false

// Whether to store failed jobs in the queue_failed_jobs table. default: false

markstory · 2022-10-24T02:50:46Z

docs/en/index.rst

        ]
    ],
    // ...

 The ``Queue`` config key can contain one or more queue configurations. Each of
 these is used for interacting with a different queuing backend.

+If ``storeFailedJobs`` is set to ``true``, make sure to run the plugin migrations to create the ``failed_jobs`` table.


Suggested change

If ``storeFailedJobs`` is set to ``true``, make sure to run the plugin migrations to create the ``failed_jobs`` table.

If ``storeFailedJobs`` is set to ``true``, make sure to run the plugin migrations to create the ``queue_failed_jobs`` table.

markstory · 2022-10-24T02:51:40Z

docs/en/index.rst

+``maxAttempts`` is configured on the job class or via a command line argument, a
+job will be considered failed if an exception is thrown while processing the job
+and there are no remaining attempts. The job will then be added to the
+``failed_jobs`` table and can be requeued manually.


Suggested change

``failed_jobs`` table and can be requeued manually.

``queue_failed_jobs`` table and can be requeued manually.

markstory · 2022-10-24T02:52:01Z

docs/en/index.rst

@@ -266,6 +276,53 @@ This shell can take a few different options:
  - Max Runtime
  - Runtime: Time since the worker started, the worker will finish when Runtime is over Max Runtime value

+Failed Jobs


Should we call this a 'dead letter queue' as that is another term that is used for this behavior.

I wonder if that will get confusing because some queue backends implement their own dead letter queues (for example, sqs, rabbitt mq). And it seems in those examples a dead letter queue is defined more broadly where here only jobs that are requeued too many times will be failed/dead lettered. Also, when a job is failed, it's not going to be queued on the original queue infrastructure, but a database table instead. I think this does fall within the concept of a dead letter queue though. Let me know what you think.

I don't disagree that the terminology would overlap with native message brokers. Using 'failed jobs' is a good term for the abstraction we're trying to provide. I think we should mention the similarities with dead letter queues when we explain the behavior.

Let's keep 'failed jobs' until we hear feedback otherwise.

I kept it as failed jobs naming and added some clarification about the similarities/differences in the documentation.

markstory · 2022-10-24T02:54:09Z

src/Command/DeleteCommand.php

+
+        if ($args->hasOption('class')) {
+            $jobsToDelete->where([
+                $failedJobsTable->aliasField('class') => $args->getOption('class'),


You shouldn't need to manually alias all of these fields. Using where(['class' => $args->getOption('class')]) should be enough.

markstory · 2022-10-24T02:54:43Z

src/Command/DeleteCommand.php

+            return;
+        }
+
+        if (!$args->getOption('assume-yes')) {


We usually call these kinds of parameters --force

markstory · 2022-10-24T02:55:01Z

src/Command/RequeueCommand.php

+        $parser->addOption('config', [
+            'help' => 'Requeue jobs by the config used to queue the job.',
+        ]);
+        $parser->addOption('assume-yes', [


Could be force as well.

markstory · 2022-10-24T02:55:27Z

src/Command/RequeueCommand.php

+
+        if ($args->hasOption('config')) {
+            $jobsToRequeue->where([
+                $failedJobsTable->aliasField('config') => $args->getOption('config'),


Same feedback here about not needing to use aliasField().

markstory · 2022-10-24T02:56:36Z

src/Command/RequeueCommand.php

+                    ]
+                );
+
+                $failedJobsTable->deleteOrFail($failedJob);


We'll only be able to do at-least once delivery with this. We should mention that in the usage docs.

You mean because if deleteOrFail fails, the job would remain in the failed_jobs table and could be requeued, right? I can note that in the docs. I suppose the only solution to that would be to remove the job from the database, queue it, then insert the record again if there was an exception when queueing. But then a failed job could be "dropped" if it could not be inserted again, which seems worse.

Yes that is the scenario. I agree that the solution is overly complicated and that we can document the delivery guarantees we currently offer. If it is a problem we can always add complexity and improve the delivery guarantees in the future.

markstory · 2022-10-30T02:51:58Z

src/Command/RequeueCommand.php

+                    ]
+                );
+
+                $failedJobsTable->deleteOrFail($failedJob);


Yes that is the scenario. I agree that the solution is overly complicated and that we can document the delivery guarantees we currently offer. If it is a problem we can always add complexity and improve the delivery guarantees in the future.

markstory · 2022-10-30T02:59:44Z

src/Consumption/LimitAttemptsExtension.php

+                ['exception' => $exception, 'logger' => $context->getLogger()]
+            );
+
+            EventManager::instance()->dispatch($event);


Should this extension use EventDispatcherTrait instead of firing the event on the global event manager directly?

I think it can actually. The listener can be attached when instantiating the extension in WorkerCommand. That's the only place it's instantiated so there isn't really any need for a global event. I'll make that change.

markstory · 2022-11-04T02:56:26Z

composer.json

@@ -20,6 +20,7 @@
    "require": {
        "php": ">=7.2.0",
        "cakephp/cakephp": "^4.1",
+        "cakephp/migrations": "^3.1",


Can we make this an optional dependency.

I made it optional and added a note in the docs to composer require it if using failed jobs.

phpstan v1.9.1 returns an error with message "Dead catch - Cake\ORM\Exception\PersistenceFailedException is never thrown in the try block.". this error was not returned on v1.8.11.

sheldonreiff · 2022-11-04T16:26:59Z

The second commit I added was to address phpstan failing. I was getting a phpstan error Dead catch - Cake\ORM\Exception\PersistenceFailedException is never thrown in the try block.. It seems version 1.9.1 introduced a change that's causing this (previous builds were using 1.8.11). I don't see anything wrong with the phpdocs.

markstory · 2022-11-04T19:25:18Z

Thank you 🎉

sheldonreiff force-pushed the failed-jobs branch 7 times, most recently from cef5041 to dbe3b8a Compare October 19, 2022 22:35

markstory reviewed Oct 24, 2022

View reviewed changes

markstory reviewed Oct 30, 2022

View reviewed changes

sheldonreiff force-pushed the failed-jobs branch 2 times, most recently from c4c8360 to fe82f67 Compare November 1, 2022 21:55

sheldonreiff requested a review from markstory November 1, 2022 21:59

markstory reviewed Nov 4, 2022

View reviewed changes

Add failed jobs and requeue/delete commands

2271a55

sheldonreiff force-pushed the failed-jobs branch from fe82f67 to 2271a55 Compare November 4, 2022 15:38

Ignore dead catch error

f07844e

phpstan v1.9.1 returns an error with message "Dead catch - Cake\ORM\Exception\PersistenceFailedException is never thrown in the try block.". this error was not returned on v1.8.11.

sheldonreiff requested a review from markstory November 4, 2022 16:27

markstory merged commit 906bc11 into cakephp:master Nov 4, 2022

sheldonreiff deleted the failed-jobs branch November 4, 2022 20:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add failed jobs support #103

Add failed jobs support #103

sheldonreiff commented Oct 19, 2022

markstory left a comment

markstory Oct 24, 2022

markstory Oct 24, 2022

markstory Oct 24, 2022

markstory Oct 24, 2022

markstory Oct 24, 2022

sheldonreiff Oct 25, 2022

markstory Oct 30, 2022 •

edited

Loading

sheldonreiff Nov 1, 2022

markstory Oct 24, 2022

markstory Oct 24, 2022

markstory Oct 24, 2022

markstory Oct 24, 2022

markstory Oct 24, 2022

sheldonreiff Oct 25, 2022

markstory Oct 30, 2022

markstory Oct 30, 2022

markstory Oct 30, 2022

sheldonreiff Oct 31, 2022

markstory Nov 4, 2022

sheldonreiff Nov 4, 2022

sheldonreiff commented Nov 4, 2022

markstory commented Nov 4, 2022

	$table = $this->table('failed_jobs');
	$table = $this->table('queue_failed_jobs');

	// Whether to store failed jobs in the failed_jobs table. default: false
	// Whether to store failed jobs in the queue_failed_jobs table. default: false

	If ``storeFailedJobs`` is set to ``true``, make sure to run the plugin migrations to create the ``failed_jobs`` table.
	If ``storeFailedJobs`` is set to ``true``, make sure to run the plugin migrations to create the ``queue_failed_jobs`` table.

	``failed_jobs`` table and can be requeued manually.
	``queue_failed_jobs`` table and can be requeued manually.

Add failed jobs support #103

Add failed jobs support #103

Conversation

sheldonreiff commented Oct 19, 2022

markstory left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

markstory Oct 30, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sheldonreiff commented Nov 4, 2022

markstory commented Nov 4, 2022

markstory Oct 30, 2022 •

edited

Loading