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

Let tasks change visibility timeout #62

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

jhorman
Copy link

@jhorman jhorman commented Jul 16, 2019

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 #47

…handling.

When a task fails, immediately make it available again instead of waiting for visibility timeout.

Fixes spulec#47
pyqs/__init__.py Outdated
@@ -1,4 +1,4 @@
from .decorator import task # noqa

__title__ = 'pyqs'
__version__ = '0.1.2'
__version__ = '0.1.4'
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need to bump this, we will do so when it is released.

@andrewgross
Copy link
Collaborator

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
Copy link
Author

jhorman commented Jul 17, 2019

  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.

… detect if the message is being sent again, b/c the message had a visibility time out on a different worker.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Dynamic Visibility Timeout
2 participants