search

seomoz / qless-core

Core Lua Scripts for qless
MIT License
85 stars 33 forks source link

Add support to pause/unpause queues. #3

Closed myronmarston closed 11 years ago

myronmarston commented 11 years ago

@dlecocq -- care to review this?

dlecocq commented 11 years ago

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.

myronmarston commented 11 years ago

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 11 years ago

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 commented 11 years ago

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.