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

add Worker#work_off and rake qc:work_off. #256

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

add Worker#work_off and rake qc:work_off. #256

wants to merge 3 commits into from

Conversation

senny
Copy link
Contributor

@senny senny commented Mar 6, 2015

This provides a way to work off all the jobs in the queue
and exit afterwards.

Closes #191.

This provides a way to work off all the jobs in the queue
and exit afterwards.

Closes #191.
@senny
Copy link
Contributor Author

senny commented Mar 6, 2015

@smathieu @jipiboily thoughts?

@rwdaigle
Copy link

rwdaigle commented Mar 6, 2015

👍

work_off seems a bit obtuse. If I could go back in history I'd propose qc:start for the daemon task and qc:work for this work-and-exit task. However, assuming we don't want to break the existing task name, I don't have a better suggestion besides something like complete, finish or clear. Actually, I think I like qc:clear best?

@smathieu
Copy link
Contributor

smathieu commented Mar 6, 2015

I would expect a task named qc:clear to delete of the tasks from the queue, not process them. I'd agree that qc:start and qc:work would be better, but unfortunately, we can't change that. I think work_off is fine for now.

@smathieu
Copy link
Contributor

smathieu commented Mar 6, 2015

The code is fine, but I worry about how this project is evolving. QC was originally designed (as far as I understand) to have a simple and limited interface for workers. This would allow users of QC to create their own subclass of the worker class and override some method.

It seems that over the year, the size of the worker interface has kept growing, making it less approachable to override.

For instance, if someone overrode the Worker#work method, he now needs to know that he should all override the Worker#work_off method. If he doesn't and he doesn't use the rake task, then it doesn't matter. But the fact of the matter is that you now have a rake task in your application that doesn't work or has unknown behaviour.

Something to think about I guess.

@senny
Copy link
Contributor Author

senny commented Mar 9, 2015

@smathieu I agree with everything you say. I also do think that this functionality should be part of the limited interface we provide.

The PR was created to be fully backwards compatible but as you pointed out, this is a tradeoff in other areas. Depending on wether this lands in a major or minor we might have more flexibility to rework the internals. I'd like to see a more approachable interface as well.

@jipiboily any thoughts on a roadmap?

@jipiboily
Copy link
Contributor

I'm on the fence on that one. I don't have a strong opinion, but it doesn't feel like something that most people will use.

The more I think about qc, the more I think it should keep a tight scope, stay easily approachable and modifiable. To achieve this, I think we should mostly add stuff that will be used by the majority of the people using qc.

Are there any other use cases you can think of? The original one (using this on Heroku to avoid paying for a full dyno) seems like something very few people will do, and it's easy to work around it.

@jipiboily
Copy link
Contributor

And by the way, my life changed a bit in the last couple months, you might have seen I don't have much time to work on qc or any open source stuff, and I don't think this will change anytime soon. Basically, don't wait for me if you want to be able to move at a decent pace :)

@senny
Copy link
Contributor Author

senny commented Jan 15, 2016

I still feel we should put this in. Some use-cases I've came across where this would be very helpful.

  • For very low-volume projects it might be easier to configure a cron-job using #work_of than to manage background processes. This would be a very easy way to get started with background jobs.
  • In integration tests when you want to run all queued_jobs before continuing without having to manage a worker process.

@jipiboily
Copy link
Contributor

I don't have a strong opinion, so either way is fine IMHO.

@bronson
Copy link
Contributor

bronson commented Feb 5, 2016

This makes sense and has precedent: delayed_job has a jobs:workoff task: https://github.com/collectiveidea/delayed_job#running-jobs

Agreed, a cron job is a good example where this feature would be needed. Another would be spinning up an expensive host twice a day to drain the queue. I'd like to see this feature.

That said, I agree with @smathieu... It's worth worrying if QC's internals are getting weirder. For example, in this PR, lock_job and lock_job_no_wait seem inside-out, and work and work_off seem like copy pasta.

Is it OK to make minor changes to lock_job and work's method signatures to clean this up? I don't imagine anyone would be overriding them.

But maybe it's not OK... Since the docs recommend overriding Worker to manage connections (!), and nobody put any private statements anywhere (!!), this project might have guaranteed that the Worker class will never change... If that's the case, I suppose this PR is the best that can be done and could be merged as-is.

@smathieu
Copy link
Contributor

smathieu commented Feb 5, 2016

@senny has convinced me that this is valuable, but I'm still not convinced this interface is the right one. Maybe we can limit the scope of change by making the lock_job method take an optional options have. e.g. lock_job(wait: false)

@ukd1
Copy link
Contributor

ukd1 commented Jul 18, 2019

@bronson @senny not sure if y'all use this any more - if you do and you wanna work on this lmk, if not I might

@ukd1 ukd1 added this to the 4.0.0 milestone Jul 23, 2019
@bronson
Copy link
Contributor

bronson commented Jul 26, 2019

I'm not in charge of any Rails projects anymore. Hope you can do it @ukd1!

@coffenbacher
Copy link

coffenbacher commented Jul 27, 2019

In integration tests when you want to run all queued_jobs before continuing without having to manage a worker process.

I'm not sure if queue_classic already addressed this in the past few years, but que has Que.run_synchronously = true which IMO is a nice solution to this.

Our Que specs use this helper:

def run_que_synchronously
  Que.run_synchronously = true
  yield
ensure
  Que.run_synchronously = false
end

it "runs jobs inline without needing a worker" do
  run_que_synchronously do
    Job.enqueue()
    OtherJob.enqueue()
  end
end

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.

Allow for transient processing of jobs
7 participants