Add support to pause/unpause queues. #3

Merged
merged 2 commits into from Jan 7, 2013

Projects

None yet

3 participants

Owner

@dlecocq -- care to review this?

@dlecocq dlecocq commented on the diff Jan 7, 2013
pause.lua
@@ -0,0 +1,18 @@
+-- This script takes the name of the queue and adds it
+-- to the ql:paused_queues set.
+--
+-- Args:
+-- 1) The queue to pause.
+--
+-- Note: long term, we have discussed adding a rate-limiting
+-- feature to qless-core, which would be more flexible and
+-- could be used for pausing (i.e. pause = set the rate to 0).
+-- For now, this is far simpler, but we should rewrite this
+-- in terms of the rate limiting feature if/when that is added.
+
dlecocq
dlecocq Jan 7, 2013 Contributor

All of the scripts do a little bit of error checking to make sure the args are sane. For this, it's pretty minimal, though:

if #KEYS > 0 then error('Pause(): No Keys should be provided') end
if #ARGV < 1 then error('Pause(): Must provide at least one queue to pause') end
myronmarston
myronmarston Jan 7, 2013 Owner

Thanks, I'll add that.

Contributor
dlecocq commented Jan 7, 2013

My only general thought is that the pause and unpause scripts are small enough that you might consider having a pause script that takes an argument on or off:

pause on queue1 queue2 queue3
pause off queue1 queue2
...

That said, there's kind of a mix of scripts in here that follow the pause/unpause-style and some that follow the pause on/pause off-style.

Owner

My only general thought is that the pause and unpause scripts are small enough that you might consider having a pause script that takes an argument on or off

I thought about doing this, but it seems wrong to make users send a "pause" command when they really want to unpause. I think it creates confusion when users have to call a method that is named the exact opposite of what they want to achieve. Makes me think of an interface like some_object.start("no, actually I meant stop") :).

If it was important to you to conslidate this in one script, I'd want it to be named something like set_paused_state or something like that...but I prefer just to keep the current separate pause/unpause scripts.

proby commented Jan 7, 2013

I think they should be consolidated into one script with a different name from pause. It makes more sense to me from a functionality point of view and it'll help down the road if/when the script gets augmented into more of a rate-limiting feature rather than a boolean on/off state.

@myronmarston myronmarston merged commit 819647e into master Jan 7, 2013
@myronmarston myronmarston deleted the pause_support branch Jan 7, 2013
Owner

I think they should be consolidated into one script with a different name from pause. It makes more sense to me from a functionality point of view and it'll help down the road if/when the script gets augmented into more of a rate-limiting feature rather than a boolean on/off state.

From an API perspective, I think set_pause_state is a far worse interface than pause/unpause, and that trumps the implementation benefit of keeping it in the same script (in my mind, at least)...particular since the scripts are so simple. If the scripts did a lot more, it would be more burdensome to keep the scripts separate and I might favor a single script.

If/when we add the rate limiting thing, we can always switch to having one script at that point if we deem it necessary.

Besides, I merged before I saw your comment. But if you care enough we can always change it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment