tarantool / queue

Create task queues, add and take jobs, monitor failed tasks
Other
236 stars 52 forks source link

Option to keep the ttl while releasing the task #149

Closed qdrin closed 3 years ago

qdrin commented 3 years ago

Adds the option 'touch_ttl' that is assotiated with delay option in the release method of fifottl. Allows keep the task ttl unchanged when it necessary. Might help with retries.

rybakit commented 3 years ago

IMHO, introducing a new option that solely depends on another option is really not the best solution to the problem. I'm afraid it will be a source of confusion for users, because the API allows the "associated" option to be passed alone and, when allowed, is likely to be misused. So I expect to see in the future something like

t:release(15, {keep_ttl=true})

which makes no sense (however, it gives a false impression that it somehow affects the task ttl). Moreover, when https://github.com/tarantool/queue/issues/125 gets implemented, we might get

t:release(15, {delay=10, keep_ttl=true, ttl=10})

which is a valid combination of options, although it looks very weird and confusing.

As an alternative, I would consider adding a new standalone option, something like delay_with_ttl or delay_wo_ttl (or any other name that works best), so at least it can be used alone. Also, we might think of forbidding passing both delay and delay_with_ttl or at least clearly define the precedence of these options.

Another alternative is to introduce a new api method.

rybakit commented 3 years ago

After thinking more about the issue, I believe it can be solved in the scope of #125. If we had the ability to pass more options to release() we could do

t:release(15, {delay=10, ttl=-1})

where ttl=-1 indicates that we don't want to update ttl.

qdrin commented 3 years ago

IMHO, introducing a new option that solely depends on another option is really not the best solution to the problem. I'm afraid it will be a source of confusion for users, because the API allows the "associated" option to be passed alone and, when allowed, is likely to be misused. So I expect to see in the future something like

t:release(15, {keep_ttl=true})

which makes no sense (however, it gives a false impression that it somehow affects the task ttl). Moreover, when #125 gets implemented, we might get

t:release(15, {delay=10, keep_ttl=true, ttl=10})

which is a valid combination of options, although it looks very weird and confusing.

As an alternative, I would consider adding a new standalone option, something like delay_with_ttl or delay_wo_ttl (or any other name that works best), so at least it can be used alone. Also, we might think of forbidding passing both delay and delay_with_ttl or at least clearly define the precedence of these options.

Another alternative is to introduce a new api method.

Perhaps the option is not the best solution. But in the current implementation it doesn't seem confusive as the only case with ttl update is the release. I agree the #125 is fine and covers much more cases. So it's implementation seems to be more complicated and i suppose deserves the particular driver. Meenwhile my purpose was to give some new uses to current one with minimum efforts.

LeonidVas commented 3 years ago

HI! Thank you for the patch. I have several questions:

qdrin commented 3 years ago

IMHO, introducing a new option that solely depends on another option is really not the best solution to the problem. I'm afraid it will be a source of confusion for users, because the API allows the "associated" option to be passed alone and, when allowed, is likely to be misused. So I expect to see in the future something like

t:release(15, {keep_ttl=true})

which makes no sense (however, it gives a false impression that it somehow affects the task ttl). Moreover, when #125 gets implemented, we might get

t:release(15, {delay=10, keep_ttl=true, ttl=10})

which is a valid combination of options, although it looks very weird and confusing.

As an alternative, I would consider adding a new standalone option, something like delay_with_ttl or delay_wo_ttl (or any other name that works best), so at least it can be used alone. Also, we might think of forbidding passing both delay and delay_with_ttl or at least clearly define the precedence of these options.

Another alternative is to introduce a new api method.

Perhaps the option is not the best solution. But in the current implementation it doesn't seem confusive as the only case with ttl update is the release. I agree the #125 is fine and covers much more cases. So it's implementation seems to be more complicated and i suppose deserves the particular driver. Meenwhile my purpose was to give some new uses to current one with minimum efforts.

* Could you to describe few use case of the feature?

My case is to retry failed tasks (http requests) every 30 seconds for 6-minutes period. So I need ttl to be constant. Shure if touch() could decrease ttl it would solve the issue along with "classic" release()

qdrin commented 3 years ago

Sorry it seems i close the request accidently. Let's me reopen

LeonidVas commented 3 years ago

My case is to retry failed tasks (http requests) every 30 seconds for 6-minutes period. So I need ttl to be constant. Shure if touch() could decrease ttl it would solve the issue along with "classic" release()

Is it close to ttr = 30 with ttl = 360. Did I understand your case correctly? Or maybe it's like ttl = 360 and release with delay = 30.

LeonidVas commented 3 years ago

Well, thanks for the PR, but it looks like we haven't decided yet whether such functionality is needed in the module and how exactly the API of this functionality should look like. I think it would be good to resolve these questions in discussion #125, and then start implementing the feature.