Remove job exception classes (and update the worker)

[juriansluiman]

See Webador/SlmQueue#58. Commit would be the same like this one from SlmQueueBeanstalkd

[svycka]

why is this still not done? maybe close if we don't want it.

[basz]

No time to investigate

[MatthiasKuehneEllerhold]

I've added the PR #140 for this issue.

This is a major BC break because classes get removed and the features (bury / release) have to be implemented in the job itself.

A new major version may be necessary. Is there some place to document the changes with examples etc?

[basz]

May an UPGRADE.md in the repo root of its more then a paragraph or two. Otherwise the readme

[basz]

Hi @MatthiasKuehneEllerhold Are able to add such documentation?

[MatthiasKuehneEllerhold]

I've ran into one problem:

Burying a job after this PR is easy: just throw any exception and it'll be buried.

Releasing one is currently impossible. Using the snippet from @juriansluiman provided in his PR:

class MyJob extends AbstractJob implements QueueAwareInterface
{
    use QueueAwareTrait;

    public function execute()
    {
        // do some work

        if (!$result) {
            $this->getQueue()->release($this);
        }
    }
}

This will release the job (correctly) and after execute() the job is marked as complete and will be deleted. See DoctrineWorker->processJob().
You could throw an exception in order to prevent the call to $queue->delete() but then the job will be buried. In each case the job is NOT rescheduled for later...

So in order to make sure that something is executed later we need to clone the job and add the clone to the queue with the scheduled date. This makes the DoctrineQueue->release() function obsolete and every user has to do this for its own.

Is this really the right way? Wouldnt it be better that every implementation (Doctrine, Beanstalk, ...) offers their own way to reschedule a job? Making release() part of the QueueInterface, adding the two exceptions to the base library and letting the implementation decide how to do this exactly.

Special cases like a failure-count (as discussed in the linked comments) can still use the QueueAware-way.

What do you think?

[MatthiasKuehneEllerhold]

(Oh and btw... I think "releasing a job" is a terrible word for it. Wouldnt "rescheduling a job" be a better fit?)

[MatthiasKuehneEllerhold]

@basz: Any thoughts?

[basz]

I think it was discussed and agreed upon that we would simply remove this functionality. It simple enough to reinsert it in userland if required.

I personally do not use it anyway. I found it confusing and don’t see much use for it. When an exception occurred it is saved with status 4 I think. Which is good enough for me. I’m not sure burying is the right approach. Same for releasing. I would rather explicitly catch and reinsert manually.

[basz]

@roelvanduijnhoven thoughts

[roelvanduijnhoven]

@MatthiasKuehneEllerhold @basz

There is an easy way to reschedule a job. We use it in production on many places. You need to throw the ReleasableException from a job.

throw new ReleasableException([
    'delay' => 6/*hours*/ * 60/*minutes*/ * 60/*seconds*/,
]);

I think it was discussed and agreed upon that we would simply remove this functionality. It simple enough to reinsert it in userland if required.

I’m not sure burying is the right approach. Same for releasing.

We use it for example to automatically reschedule failing external APIs. It does work pretty well for that! I think I missed the earlier discussions about this. @basz hat would be the pros to drop this exception based API? Re-inserting a job by manually cloning / adapting it seems like a lot of work!

[basz]

I only quickly read up on this, and might have overlooked something. After rereading a bit I now believe this shouldn't be about removing this functionality completely as a feature. It should mean these classes are now implemented on the main package and can be removed from here. It should still be possible to release/bury. Just with different exceptions from a different namespace. I think this block should then stay (with different namespace): 4641801#diff-11887d92b071bf14d7c9fd9e86728846L32

So in theory no features are lost... Does this makes sense?

[MatthiasKuehneEllerhold]

Yeah, that's what I'd prefer too. But that would mean, that each implementation (SlmQueueDoctrine, SlmQueueBeanstalkd, ...) has to implement this feature too. And AFAIK SlmQueueBeanstalkd has removed it in an earlier commit?

[roelvanduijnhoven]

@basz @MatthiasKuehneEllerhold Yes that aligns with my thoughts. We can create an issue to track that progress on the main repo.

In the meantime: I guess it is not clear how this function works now because there is no documentation in this repo. So may be a PR in the meantime to fix that?