Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add an ability to customize task options (and data) when calling tube:release() #125

Open
rybakit opened this issue Jul 10, 2020 · 5 comments
Labels
feature A new functionality

Comments

@rybakit
Copy link
Contributor

rybakit commented Jul 10, 2020

tube:release() accepts an optional second argument opts, which at this moment can only consist of delay. But often when I release a task back into the queue, I need to adjust other task options, such as priority or ttl. Also, in task data I usually store additional options such as retry strategy, retry count, etc, and it would be great if I can adjust them as well when calling tube:release() (or even better, add an extra field called extra or meta where users can store custom task options?).

@LeonidVas
Copy link
Contributor

What do you think about implementing the task "options" update in a separate method like touch?

@rybakit
Copy link
Contributor Author

rybakit commented Apr 23, 2021

What do you think about implementing the task "options" update in a separate method like touch?

@LeonidVas Could you elaborate on how you see it, what options will it support? Maybe you have some examples on hand?

@LeonidVas
Copy link
Contributor

What do you think about implementing the task "options" update in a separate method like touch?

@LeonidVas Could you elaborate on how you see it, what options will it support? Maybe you have some examples on hand?

Respond to question with counter-questions - not bad)(Вы, таки, не с Одессы будете?)
Frankly, I started thinking about this task after you refer to it in #149 .
Now I can only share my raw thoughts on this theme:

  • I don't know if this is a true way to change input parameters of a task (although we already have the touch method that can affect ttl).
  • release called by the worker and IMHO it is outside of his competence to change the input parameters of a task.
  • Now we already have a method which can affect the input parameters of a task(touch), and maybe it would be logical to expand functionality of this method instead of adding such functionality to the release method.

For ascertaining the answers, I should to look around and see how this works in other similar software, and when I asked you a question, I thought you have some knowledge in this area and you would like to share it.

@rybakit
Copy link
Contributor Author

rybakit commented Apr 23, 2021

OK, let me explain then why I asked you about giving more details :) When you proposed the new api method, I thought you had already thought about how it would work and what it should accept. For example, will it be able to increment, decrement, or reset tasks ttr:

t:options(task_id, {ttr_inc=60})

Will it allow to change the task state:

t:options(task_id, {state=state.READY})

or/and delay:

t:options(task_id, {delay=42})

If so, what do you plan to do with the existing functions that already have this functionality? Deprecate them or keep them?
If not, what options do you actually propose to allow with this new function? Also, what about the custom options that I mentioned in the description? Will it support them and in which form?

See, there are tons of questions and just throwing random ideas without specifying exactly what you meant doesn't help, no one can read your mind. Without knowing these details, I can't give any meaningful answer. But if you're expecting a super generic answer: no, I don't really like it, and I don't remember seeing anything like that in any other related product. At a glance, it feels too low-level end error-prone, because you can't make those option arguments part of the interface (if this term can be applied to lua :)). Also, I don't know what problem you are trying to solve by introducing a new function. If it's about keeping release() from getting too bloated, maybe it's better to introduce release_with_opts()? So many questions :D

@rybakit
Copy link
Contributor Author

rybakit commented Apr 25, 2021

Now I can only share my raw thoughts on this theme:

Thanks for that! (I wish you did it from the beginning :))

I don't know if this is a true way to change input parameters of a task (although we already have the touch method that can affect ttl).

Neither do I. Please see my thoughts below.

release called by the worker and IMHO it is outside of his competence to change the input parameters of a task.

Good point. And I don't have a clear answer for that. It could depend on how we treat these input parameters. Of course, if they were "domain" parameters like user_id or deposit_amount I would fully agree with you. But when I look at the list of currently available task options, they are all worker-specific, and I don't see how they can be used elsewhere. So maybe it's more about user responsibility and awareness that such options can be modified by the worker? Or maybe they can be guarded with a "read-only" flag or something to prevent "implicit" modification, Idk :)

Now we already have a method which can affect the input parameters of a task(touch), and maybe it would be logical to expand functionality of this method instead of adding such functionality to the release method.

From the usability perspective, if I had to choose between

t:touch(task_id, 60);

and

t:options(task_id, {ttl={'+', 60}})

I would choose the former just because it reads better and I don't have to check the option name and how to increment its value in the docs. On the other hand, having the ability to modify multiple options at once could be useful when used from synchronous connectors (see #124).

FTR, If you decide to go with the options() approach, it will not solve my original issue, as I would have to call the two functions together all the time:

t:release(task_id)
t:options(task_id, {ttr={'+', 60}, prio={'+', 1}, meta={ retry={'+', 1} }})

which means that I have to extend this library by adding my own Lua function that wraps these calls. This completely defeats the purpose of this ticket, as I already have such a wrapper that I would like to get rid of.

After all, I'm more in favor of extending release() than adding options(). I don't think it's worth introducing a new function because it would require deprecating the second argument of release() and touch(). But if you decide to keep it, it would mean duplicating functionality which is another source of confusion for users. And as I already mentioned, I simply don't have a use case where I would use options() independently of release() (and btw, I don't have a use case for touch() either).

@LeonidVas LeonidVas removed backlog Issues for upcoming releases (to be done soon) prio5 labels Dec 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature A new functionality
Projects
None yet
Development

No branches or pull requests

4 participants