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

Deferring a periodic job to multiple queues does not work as expected #289

Closed
elemoine opened this issue Aug 10, 2020 · 4 comments · Fixed by #296
Closed

Deferring a periodic job to multiple queues does not work as expected #289

elemoine opened this issue Aug 10, 2020 · 4 comments · Fixed by #296
Labels
Issue appropriate for: Occasional contributors 😉 This issue will be best tackled by people with a minimum of experience on the project Issue contains: Some documentation 📚 This issues involves writing some documentation Issue contains: Some Python 🐍 This issue involves writing some Python code Issue contains: Some SQL 🐘 This features require changing the SQL model Issue type: Bug 🐞 Something isn't working

Comments

@elemoine
Copy link
Contributor

Let's say I want per worker server periodic tasks. Naturally I'd be tempted to use per-server queues:

@app.periodic(cron="*/5 * * * *")
@app.task(queue=host_settings.queue_name)
def my_task(timestamp):
    …

But, as I understand it, the procrastinate_defer_periodic_job function will refuse to defer a periodic job if a periodic job with the same name and same defer timestamp was already deferred, even if the periodic job is not deferred to the same queue as the one already deferred.

Shouldn't the procrastinate_periodic_defers table include a queue_name column? And shouldn't the unique constraint be UNIQUE (task_name, queue_name, defer_timestamp) rather than UNIQUE (task_name, defer_timestamp)?

@elemoine elemoine added the Issue type: Bug 🐞 Something isn't working label Aug 10, 2020
@ewjoachim
Copy link
Member

ewjoachim commented Aug 11, 2020

It's loosely linked to #260 I'd say.

As of now, the interaction between queue and task name is not always clear. In your example, I think what was imagined was a different task name AND queue. But you're right, we could improve this (and your solutions seems A-OK)

Generally, it looks like anywhere a "task" needs to be unambiguously identified, the combination of queue and task name should be used (whereas the task name alone should only be used to find the corresponding callable to launch)

@elemoine
Copy link
Contributor Author

Great summary @ewjoachim!

@ewjoachim ewjoachim added Issue contains: Some SQL 🐘 This features require changing the SQL model Issue contains: Some documentation 📚 This issues involves writing some documentation Issue contains: Some Python 🐍 This issue involves writing some Python code Issue appropriate for: Occasional contributors 😉 This issue will be best tackled by people with a minimum of experience on the project labels Aug 18, 2020
@ewjoachim
Copy link
Member

This could maybe be "Good for newcomers" too but I'm afraid there are too many unclear things.

@ewjoachim
Copy link
Member

Anyone browsing this ticket might want to have a look at https://github.com/procrastinate-org/procrastinate/releases/tag/0.23.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Issue appropriate for: Occasional contributors 😉 This issue will be best tackled by people with a minimum of experience on the project Issue contains: Some documentation 📚 This issues involves writing some documentation Issue contains: Some Python 🐍 This issue involves writing some Python code Issue contains: Some SQL 🐘 This features require changing the SQL model Issue type: Bug 🐞 Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants