Idempotent queue pause and resume #408
Conversation
| UPDATE river_queue | ||
| SET | ||
| paused_at = now(), | ||
| paused_at = coalesce(queue_to_update.paused_at, now()), | ||
| updated_at = now() | ||
| FROM queue_to_update | ||
| WHERE river_queue.name = queue_to_update.name; |
There was a problem hiding this comment.
To avoid editing a row for no reason, we could take an approach we've done in some other queries: do the update in a CTE, and then for the final return do a union w/ the updated row and original row to return only one of them.
Not a high throughput query so it doesn't matter much, but it would be a nice tweak to stay consistent in our approach.
There was a problem hiding this comment.
Okay, good call. Updated.
brandur
force-pushed
the
brandur-idempotent-pause-or-resume
branch
from
4beef49 to
7543bbe
Compare
|
@bgentry Actually, while looking through that query code I found one other related problem: when you pause/resume using the all queues This also feels a bit wrong to me, so I changed it to be a no-op. Do you agree with that behavior? If so, maybe take one more review pass since I added some more code and test cases. |
|
Yes, I agree, a no op seems better in that case! |
I was writing tests for River UI and found that when trying to pause an already paused queue or resume an unpaused queue, River returns a "not found" error. This was quite surprising, so much so that it took me a good half hour of debugging before I considered the fact that it might actually be a problem in upstream River. Even if an argument were to be made that pausing an already paused queue should be an error like "queue already paused", returning "not found" is misleading and guaranteed to result in confused people beyond just myself. I think it's fine for pause and resume to be considered idempotent operations. If a paused queue is paused again, there's no real damage done, especially if we keep the original paused time, which we do in this implementation. Similarly, make an update so that when pausing or resuming using the all queues string (`*`), don't return "not found" error if there are no queues in the database. Instead, just no-op.
brandur
force-pushed
the
brandur-idempotent-pause-or-resume
branch
from
7543bbe to
52b6144
Compare
|
Great, thanks! Also added changelog. |
I was writing tests for River UI and found that when trying to pause an
already paused queue or resume an unpaused queue, River returns a "not
found" error.
This was quite surprising, so much so that it took me a good half hour
of debugging before I considered the fact that it might actually be a
problem in upstream River. Even if an argument were to be made that
pausing an already paused queue should be an error like "queue already
paused", returning "not found" is misleading and guaranteed to result
in confused people beyond just myself.
I think it's fine for pause and resume to be considered idempotent
operations. If a paused queue is paused again, there's no real damage
done, especially if we keep the original paused time, which we do in
this implementation.
Similarly, make an update so that when pausing or resuming using the
all queues string (
*), don't return "not found" error if there are noqueues in the database. Instead, just no-op.