search

Webador / SlmQueue

Laminas / Mezzio module that integrates with various queue management systems.
Other
137 stars 56 forks source link

Return JobStatus value object from Job to update job's state #143

Closed bgallagher closed 2 years ago

bgallagher commented 10 years ago

Reading the documentation on Job Status Codes (https://github.com/juriansluiman/SlmQueue/blob/master/docs/3.Jobs.md#job-status-codes) it indicates that Jobs can return a status code. However from reading through the worker code it would seem like a return value from the Job is never evaluated? See https://github.com/juriansluiman/SlmQueueSqs/blob/master/src/SlmQueueSqs/Worker/SqsWorker.php#L31 - (same for the other workers).

Two confusing point in the documentation are:

  1. "If any exception is thrown, the status is set to failure." It would seem that if the job throws any exception the status code is set to recoverable. Is it possible to indicate a fatal job failure? eg https://github.com/juriansluiman/SlmQueueBeanstalkd/blob/master/src/SlmQueueBeanstalkd/Worker/BeanstalkdWorker.php#L37
  2. "...you can return a non-NULL value from the Job's execute() method" Like I discussed above - it would appear that the returned codes are ignored?

Maybe I'm missing something? I looked at ProcessQueueStrategy but it seems to use the status code returned by the worker, not the job.

juriansluiman commented 10 years ago

First, I totally agree with your comments. Secondly, it was a named feature but never implemented in SlmQueue v0.4.0.

For now, I had made a POC of the feature for Beanstalkd juriansluiman/SlmQueueBeanstalkd#38 and we discussed it slightly. So the docs are likely already written for version v0.5 ;)

Your point about exceptions: I am not sure how we deal with that at the moment to be honest. Ping @bakura10 @basz perhaps they have more insights in the whys and whens of exceptions in jobs.

bakura10 commented 10 years ago

Yeah I'm not satisfied at all by the exception handling currently. Basically, we catch the very generic \Exception and reinsert the job. I think we should introduce a generic "RecoverableException", so we could catch exceptions we KNOW are "recoverable".

juriansluiman commented 10 years ago

Michael, we had this before, but it wasn't ideal too. E.g. for beanstalk, you can bury the job with a priority. To deal with this in exceptions, there was an additional parameter set in the exception. And controlling behaviour by exceptions is uhm, "not ideal" ;-)

I agree we need to come up with an improved solution, although I really don't know how to deal with it without complicating SlmQueue even more.

basz commented 10 years ago

Doctrine can do a BuryableException (bury job in queue with special status, so it can be recovered via an controller action) and ReleasableException (reinsert job), never use both of them. Both with their own options. horrible... :-)

bgallagher commented 10 years ago

It would seem like Jobs returning status codes to the worker like proposed in https://github.com/juriansluiman/SlmQueueBeanstalkd/pull/38 would be a solution? From what I see it wouldn't complicate SlmQueue any more but instead add extra responsibility to the individual workers.

What if the jobs optionally return status value objects - eg SuccessStatus, RecoverableStatus, FailureStatus - where needed each queue implementation module could extend these Status value objects and set their own requirements eg priority. I realise that could couple the job to the queue implementation but without some custom extended classes like this I think you'll lose features of the underlining queue such as bury/kick/release in the case of Beanstalkd. Am I right in saying that these functions still exist, but are no longer in use?

juriansluiman commented 10 years ago

I like the value object more than the constants as status code :)

The idea of status codes was to compare them bitwise for feature flags like statuses. With the encapsulation of status code VOs you could apply more information which creates a sustainable feature; for other (new) adapters or new features in the existing adapters you do not need to change the codes, only the adapter specific status codes.

However, this is (again...) a BC break for listeners relying on integer values. This means we have to tag it with v0.5 when we release the feature. @basz @bakura10 thoughts?

roelvanduijnhoven commented 6 years ago

The return status of jobs is thus not used; but the docs state that they do.

We could release the next major version; and require plugins to update to make sure they'll correctly handle the status returned by a job's execute method. The next Doctrine release could drop the exception mechanism and use the Value objects returned.

However in order for this to work the value options should have an optional $options array parameter. Such that in Doctrine we can still allow the user to return a JobStatus::recovery(['delay' => 3600]) or something alike.

Thoughts on this @basz?

basz commented 6 years ago

My thoughts;

roelvanduijnhoven commented 6 years ago

@basz In what way is it related to #17? I miss that link.

Does any of you see any gains / wins if we were to refactor this? I mean it is a BC; so you should hope that this does enable some new behaviour. The original issue is fixed by simply updating the docs.. :thinking:

basz commented 6 years ago

rereading i don't now exactly why #17 would be related. 'whateverspecifictosomeadapted' feels like an interface defined by the adapter. so nvm...

I also don't see any immediate feature benefits from refactoring (good point though!). So I guess a change like discussed will mainly mean a better thought out API, which might lead to more features in the future. But honestly the thing work as is for me...

roelvanduijnhoven commented 6 years ago

I also don't see any immediate feature benefits from refactoring (good point though!). So I guess a change like discussed will mainly mean a better thought out API, which might lead to more features in the future. But honestly the thing work as is for me...

Thanks for the input. I resolved this issue for now by removing the relevant part from the docs via https://github.com/JouwWeb/SlmQueue/commit/23244fee24f841dbff848aaf126d4f67a42aee9d.

Let's keep this open as an enhancement for now!

roelvanduijnhoven commented 4 years ago

I am myself updating one of my applications, and some things are bothering me now, that are related to this issue. I'll reopen this one.

With the introduction of type we now need to do this to get a proper job:

public function execute(): ?int
{
    [..]
    return null; // <-- mandatory!
}

Which is quite painful for several reasons:

Solution A: remove interface

So: one way forward is to change the return signature to void. We bump a major version.

We could also seize this moment to change some other things mentioned here. To make stuff like releasing or burying jobs possible we could either:

Solution B:

From the discussion I can read that nobody likes the exception mechanism (as used in slm/queue-doctrine). So.. I guess we should not do that.

Solution C:

I think value objects are thus the solution.

Properly using types we get:

public function execute(): JobResult
{
    [..]
    new JobSuccess();
}

Solution D:

However.. I don't like the fact that you need to return a return value if a job did it's thing. I therefore propose that we don't use the type mechanism for the return value. We obviously still throw an exception if we receive some unexpected return type.

To sum it al up, using solution D you could use something like this:

public function execute()
{
    [..]
    if (2 > 4) {
        return new RetryJob();
    }
}

Curious to hear what you think @basz?

roelvanduijnhoven commented 2 years ago

I'll close since there has been no activity lately. Obviously feel free to open a PR with such a feature :).