search

spulec / PyQS

Python task-queues for Amazon SQS
MIT License
174 stars 36 forks source link

Let tasks change visibility timeout #62

Open jhorman opened 5 years ago

jhorman commented 5 years ago

Allow tasks to change the visibility timeout of the message they are handling. When a task fails, immediately make it available again instead of waiting for visibility timeout.

Fixes https://github.com/spulec/PyQS/issues/47

andrewgross commented 5 years ago

Hey jhorman,

Thanks for submitting this! Will give it a more thorough review shortly. Couple questions in the meantime:

  1. It looks like failed messages with always become available immediately. Is this intended, or do you want to make it configurable? I don't think we want to unilaterally change visibility timeout on task failure, but I think it would be useful to add the ability to configure this.

  2. Do you think its necessary to have the task accept the args, or just pass it as a kwarg for all tasks? We could also try to find a way to add in to the function scope, without needing to modify the function signature. For something as simple as just changing the visibility timeout, I am not sure it would be best to inject a context for the processing task, instead of giving a more configuration oriented approach to rescheduling failed tasks.

jhorman commented 5 years ago

  1. We can make the failure visibility timeout configurable. The problem I am trying to solve for is that my queues are configured for a 10 minute default timeout, which on failure is a long time to wait.

  2. I didn't want to always pass context since that it would break all existing tasks, forcing them to accept the arg.

I suppose, since this code is single threaded that yeah, we could set something like function_name.context. That is how celery used to work, but now celery seems to pass in self.

http://docs.celeryproject.org/en/latest/userguide/tasks.html#example. They only pass self if you set bind=true on @task, I think.

I guess to match celery we could do the same. @task(bind=true), and have that then pass the first arg as the context.

I didn't want to rely on config since the whole idea of this is that I have tasks with unknown completion time. I want to be able to renew my lease on the task dynamically/periodically.

I think the idea of context can be expanded as well. Allowing access to the message id, other sqs functions, timing information, there could even be task counts, etc, retry control.