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

[Proposal] Method to release a job back to the queue without increasing attempts ("pause" specific job) #735

Closed
jpmurray opened this issue Aug 12, 2017 · 19 comments

Comments

@jpmurray
Copy link

@jpmurray jpmurray commented Aug 12, 2017

After searching a bit and finding this laravel/framework#18288, I wonder if there isn't a place for a method that would release the job back to a queue without increasing the number of attempts (well, I know I'd love to see that!). Let's call it pause() for the sake of discussion.

While release() effectively says "retry in X time", a pause() method would mean "let's wait X time to try again". That pause() method would probably just change the delay time of the job without changing the number of attempts.

My own use case revolve around some jobs that are starting external task that take some time to complete. Ex.:

  • Job 1 starts, start a task externally, then dispatch job 2;
  • Job 2 starts, checks if the task is finished, if not, pause() for 15 seconds;
  • After 15 seconds, job is starting again, checks if the task is finished, if it's not, pause again, if it is, do it's thing.

For now, the only effective way I've found of emulating this kind of behavior is for "Job 2" to delete itself and redispatch a new "Job 2", which feels weird to me.

Thoughts? Maybe I completely missed some other way to do it, I'd be glad to hear them!

@drbyte

This comment has been minimized.

Copy link

@drbyte drbyte commented Aug 12, 2017

In the context you describe I wonder if "delay" might be a better word than "pause". (In my mind "pause" says "hold until I tell you to un-pause".)

Just my 2 cents to the discussion.

@jpmurray

This comment has been minimized.

Copy link
Author

@jpmurray jpmurray commented Aug 12, 2017

Probably right, @drbyte.

Although there is already a delay method on the queuable trait, it might be confusing?

@sisve

This comment has been minimized.

Copy link

@sisve sisve commented Aug 13, 2017

This is already supported by the InteractsWithQueue trait and the release($delay = 0) method.

@drbyte

This comment has been minimized.

Copy link

@drbyte drbyte commented Aug 13, 2017

@sisve Yes! I thought I'd seen that, but couldn't remember for sure.

@jpmurray

This comment has been minimized.

Copy link
Author

@jpmurray jpmurray commented Aug 13, 2017

@sisve @drbyte I am using the release method and if, for example, Horizon is set to tries => 3, and I release(15) (in my case) the job 3 times, it goes straight to failed jobs. Maybe I'm misunderstanding the usage, but here, it does seems increment the number of attempts.

@sisve

This comment has been minimized.

Copy link

@sisve sisve commented Aug 14, 2017

Okay, so this seems to have changed in 5.2.40 with laravel/framework@c7975cb that changes markJobAsReserved to not only mark a job as reserved, but also increase the attempts counter. The release method does what it should do, the problem is in markJobAsReserved.

A solution would be to write a custom DatabaseQueue implementation and change the behavior of the attempts column.

@jpmurray

This comment has been minimized.

Copy link
Author

@jpmurray jpmurray commented Aug 14, 2017

@sisve you got me looking in the right direction. On my end, I'm working with redis, and it seems there is a resetAttempt method, in the queue:retry artisan command that applies to Redis jobs... I'll be looking deeper in the coming hours (or tomorrow), but I wonder how I pertinent it could be to just go and and export that method to the RedisQueue class...

@ohnotnow

This comment has been minimized.

Copy link

@ohnotnow ohnotnow commented Feb 17, 2018

Just an extra voice to say I'd quite like to see this too. I've recently had to do some queued jobs which were dependent on other external tasks and ended up having to do a 'add another boolean to the table which is then checked by a scheduled task which then fires the job if someCondition()' - but it would be way nicer (and more obvious) to be able to do something on the job itself like if (someCondition()) { $this->requeue(5); }.

@jpmurray did you get anywhere on this - I can help if needed :-)

@sisve

This comment has been minimized.

Copy link

@sisve sisve commented Feb 17, 2018

$this->dispatch($this->delay(5));

This takes the existing job, sets the delay to 5 seconds and dispatches it again. This means, technically, that it is not the same job that is released back. It's a new instance (a new row in the DatabaseQueue's jobs table), and the existing job is thought of as executed successfully.

I have a few long-running jobs (chunk through the entire database, takes many hours), and the job stores the state in class properties. This means that I can dispatch it, and it will be considered as a new job, but start executing where the previous left off. I do this to avoid timeout issues if a single job has executed for too long.

$this->lastId = last($identifiers);
$this->dispatch($this);
@ohnotnow

This comment has been minimized.

Copy link

@ohnotnow ohnotnow commented Feb 21, 2018

@sisve ah - that's not bad - I can probably live with just adding that as a requeue() method on the job for a bit of readability :-) thanks for the pointer :-)

@taylorotwell

This comment has been minimized.

Copy link
Member

@taylorotwell taylorotwell commented Mar 13, 2018

I'm fine with the general idea but don't like pause name.

@sisve

This comment has been minimized.

Copy link

@sisve sisve commented Mar 13, 2018

Are you even reading through the issue before closing them? How could the name of a method be so severely blocking that you have to close the issue?

@barryvdh

This comment has been minimized.

Copy link

@barryvdh barryvdh commented Apr 3, 2018

I think he means you can create a PR with the functionality, but think about the name some more ;)

On topic; seems relevant for me also, especially when combining with the Throttle functionality.

@jimmerioles

This comment has been minimized.

Copy link

@jimmerioles jimmerioles commented Apr 4, 2018

I think @taylorotwell should use more emojis in his replies, his tone can be easily misunderstood.

i.e.:

I'm fine with the general idea but don't like pause name.

vs

I'm fine with the general idea 👍 but don't like pause name. 🤔

 

Sorry to pollute this thread, should I post this as another issue in laravel/ideas? 😄

@eelcol

This comment has been minimized.

Copy link

@eelcol eelcol commented Sep 5, 2018

Is this proposal processed yet? Also looking for a way to release a job back onto the queue, without increasing the numtries variable.

@Artistan

This comment has been minimized.

Copy link

@Artistan Artistan commented Sep 27, 2018

Here is an example to ensure it gets requeued on the same connection and queue

Laravel 5.6


<?php

namespace App;

use Illuminate\Bus\Queueable;
use Illuminate\Contracts\Queue\ShouldQueue;
use Illuminate\Foundation\Bus\Dispatchable;
use Illuminate\Queue\InteractsWithQueue;
use Illuminate\Queue\SerializesModels;

class AJob implements ShouldQueue
{
    use InteractsWithQueue, Dispatchable, Queueable, SerializesModels;

    /**
     * @var array
     */
    private $params;

    /**
     * AJob constructor.
     *
     * @param $param1
     * @param $param2
     */
    public function __construct($param1, $param2)
    {
        $this->params = func_get_args();
    }

    public function handle()
    {
        try {
            /**
             * going to do a lot of logic here or throw exception that may cause it to requeue
             */
            throw new \Exception('Requeue',5);
        } catch (\Exception $exception) {
            // check for "Requeue" and dispatch a new job. Uses $exception->getCode() for the delay seconds :)
            if($exception->getMessage() === 'Requeue') {
                $this->dispatch(...$this->params)->onConnection($this->connection)->onQueue($this->queue)->delay($exception->getCode());
            }
        }
    }
}

@shawnhind

This comment has been minimized.

Copy link

@shawnhind shawnhind commented May 30, 2019

I just wanted to mention a problem I encountered using the approach that @Artistan suggested.

I needed to use this for handling throttled / funneled jobs, I didn't want being throttled or funneled to effect the number of retries, so i recreated the job, however this actually resets the current number of retries as well (which I didn't realize). So if a job was failing for a valid reason, and increasing the number of retries, and then got throttled, funneled in between subsequent failed attempts the job would keep being attempted and not get marked as failed.

I'm currently looking for a way to pass the number of current attempts along with the newly created job.

@Artistan

This comment has been minimized.

Copy link

@Artistan Artistan commented May 31, 2019

@shawnhind - you should be able to just have a , $param3 that is your counter.

  • pass in total retries for the initial queued job.
  • decremented it each time you run it. $this->params[2]--;
  • when you $this->dispatch(...$this->params) it will have the decremented retries value.
@shawnhind

This comment has been minimized.

Copy link

@shawnhind shawnhind commented May 31, 2019

@Artistan thanks, not sure why I hadn't thought of that, that's an easy solution. I had started digging into how I could decrement the tries by extending the laravel classes to add a method for it, but once I started seeing Lua scripts to execute the redis calls, I realized that this would be too much of a pain to maintain when updating laravel.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
10 participants
You can’t perform that action at this time.