I think we should look for way more than 1 task at a time. Because right now, the way it's implement makes it so that every task:

1. adds a 1 second delay
2. makes an SQL request to the hostmaster database

We can improve on that. I'm not sure I see a reason why the dispatcher whould just grab on to the whole queue and process all the tasks it can find in it in serie.

The only thing I can think of is that this could potentially lead to some tasks being executed twice if they are run manually using hosting-task by an admin. But then they would be shooting themselves in the foot and they may actually *want* to do that, so I would let them.

Besides, 2.x now has needs --force to run tasks that are not queued, so that would make that problem go away completely.

I would bump up that number to some arbitrary ridiculous amount like 100 or something. That means we should check the timeout within the inner loop too however...

Comments

anarcat’s picture

anarcat commented
Status: Active » Needs review

I pushed a "moretasks" branch for review.

steven jones’s picture

steven jones commented

For complicating the code is it really worth it?

Most of the time and for most people, I suspect that there would only be one task in the queue at a time that needs executing. I'll admit that there are cases where you can quite easily get more tasks in the queue, but then it's only a very simple DB query and only an one second delay.

anarcat’s picture

anarcat commented

Maybe we can make this customizable then?

steven jones’s picture

steven jones commented

But that would be more code. I liked the daemon because, actually it was really simple to understand. We've added the auto-restart stuff and that has complicated the code there, and now we're talking about complicating the simple loop too. I think there's value in keeping things simple as well as keeping them useful to all users, so I'm not totally against it, but wary of it.

anarcat’s picture

anarcat commented

I understand. If you actually look at the code, it's really not that much more complicated - we have one more conditionnal check and one less argument to a function call:

http://drupalcode.org/project/hosting_queue_runner.git/commitdiff/443c8a...

hardly more complicated. Besides, I think we could use the settings from the general Aegir queue settings instead of creating a new one here...

Maybe we can keep this for the 2.x merge though.

But note that fetching more tasks actually *won't* change anything for "1 task per run" users - the function fetches a *maximum* of "N" functions...

steven jones’s picture

steven jones commented

Okay okay, I might be almost convinced, I've updated the branch to have the latest code from 6.x-1.x. I will have a test and a play and maybe we'll merge it in.

steven jones’s picture

steven jones commented
Status: Needs review » Fixed

Merged it all in. Nice work.

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.