Implements a task scheduler for resumable potentially long running tasks #15891

MatMaul · 2023-07-07T15:42:05Z

The main purpose is to have resumable tasks, with state stored in the DB and the tasks being resumed after a synapse restart.

This is intended to run potentially long running tasks, and be able to schedule them roughly in the future.
This is NOT intended to precisely schedule tasks. The loop is currently running every minute so this is the max granularity we can have, and tasks can be postpone if the running queue is already too big.

This is kind of a spinoff of #15488 following the reviews, which currently implements something similar in a non generic way. I plan to rebase it on top of this framework.

Pull Request Checklist

Pull request is based on the develop branch
Pull request includes a changelog file.
Pull request includes a sign off
Code style is correct
(run the linters)

clokep · 2023-07-12T11:38:37Z

Out of curiosity -- is there a hope / plan that it might be useful for other tasks in the future? Should we be moving other things to this?

clokep · 2023-07-14T18:08:53Z

changelog.d/15891.feature

@@ -0,0 +1 @@
+Implements a task scheduler for	resumable potentially long running tasks.


I don't think this needs to be a feature -- it is something internally used by Synapse; admins and end-users don't care.

We could probably just combine this with the other PR?

synapse/types/__init__.py

synapse/storage/databases/main/task_scheduler.py

synapse/storage/schema/main/delta/79/03_scheduled_tasks.sql

synapse/util/task_scheduler.py

clokep · 2023-07-14T18:39:52Z

I'd find it very helpful to see how this would be used in the other PR!

synapse/util/task_scheduler.py

MatMaul · 2023-07-17T13:06:06Z

Out of curiosity -- is there a hope / plan that it might be useful for other tasks in the future? Should we be moving other things to this?

Background DB updates could (should ?) probably be port to this later on. I haven't checked the details of that however.

I'd find it very helpful to see how this would be used in the other PR!

I am working on this right now since it is also a nice way to validate this works and is enough at least for this use case. This PR will probably get a follow-up update following me trying a real use case.

clokep · 2023-08-08T19:34:31Z

I'm putting this back in the overall review queue as it is a hefty chunk of code that I think could use a secondary review.

erikjohnston

I think most of this seems relatively sane, but is missing a big docstring somewhere about how it all works? It might also help if you could point to a usage of this to see how it is to use?

erikjohnston · 2023-08-11T09:36:49Z

synapse/storage/schema/main/delta/80/01_scheduled_tasks.sql

+    params TEXT,
+    result TEXT,
+    error TEXT
+);


Do we want to add some indices for the various fields here? Or is the assumption that this is going to be low volume?

It should be low volume. But then an indice doesn't cost much in space so perhaps we should still do it ?
I am going to add one at least on status, since this one is used in the reconcile loop and run every 5mn.

resource_id and action would help for get_tasks but I feel they are perhaps less important.

MatMaul · 2023-08-11T10:43:15Z

I think most of this seems relatively sane, but is missing a big docstring somewhere about how it all works?

Big docstring is for today ;)

It might also help if you could point to a usage of this to see how it is to use?

My purge PR has been updated a while ago to use it.

…th no updates

MatMaul · 2023-08-14T10:45:36Z

The non-resolved comments are waiting for ACK/feedback, I think we should be really close to merging :)

erikjohnston

I think this works, just have a query around worker support and a suggestion for improving the DB API a bit

synapse/util/task_scheduler.py

synapse/storage/schema/main/delta/80/02_scheduled_tasks.sql

synapse/storage/databases/main/task_scheduler.py

synapse/util/task_scheduler.py

synapse/storage/databases/main/task_scheduler.py

erikjohnston

Woo!

synapse/storage/databases/main/task_scheduler.py

asked by the author :)

I don't think has caused any actual issues. Introduced in #15891

MatMaul force-pushed the mv/task-scheduler branch 2 times, most recently from 1c980d4 to 7c49b15 Compare July 7, 2023 16:05

MatMaul mentioned this pull request Jul 7, 2023

Add automatic purge after all users forget a room #15488

Merged

4 tasks

MatMaul force-pushed the mv/task-scheduler branch 11 times, most recently from 7a2a044 to 1d29dcd Compare July 10, 2023 13:29

MatMaul added the Z-Time-Tracked Element employees should track their time spent on this issue/PR. label Jul 10, 2023

MatMaul force-pushed the mv/task-scheduler branch 2 times, most recently from d37e2ec to 527d442 Compare July 11, 2023 13:46

MatMaul marked this pull request as ready for review July 11, 2023 13:46

MatMaul requested a review from a team as a code owner July 11, 2023 13:46

MatMaul changed the title ~~Implements a task scheduler~~ Implements a task scheduler for resumable potentially long running tasks Jul 11, 2023

MatMaul marked this pull request as draft July 11, 2023 14:18

Implements a task scheduler for resumable potentially long running tasks

470f385

MatMaul force-pushed the mv/task-scheduler branch from 527d442 to 470f385 Compare July 11, 2023 14:26

MatMaul marked this pull request as ready for review July 11, 2023 14:55

clokep previously requested changes Jul 14, 2023

View reviewed changes

clokep reviewed Jul 14, 2023

View reviewed changes

synapse/util/task_scheduler.py Outdated Show resolved Hide resolved

clokep reviewed Jul 14, 2023

View reviewed changes

synapse/util/task_scheduler.py Show resolved Hide resolved

Mathieu Velten added 2 commits August 10, 2023 13:47

Address comments

16be76b

Address comments

ad25237

erikjohnston mentioned this pull request Aug 10, 2023

Delete rows from device_inbox in the background when deleting a device #16098

Closed

erikjohnston reviewed Aug 11, 2023

View reviewed changes

Mathieu Velten added 2 commits August 11, 2023 12:16

Add delete_task

7b96379

Add index on status

3fd518c

Mathieu Velten added 2 commits August 14, 2023 12:17

Add big docstring

1a410ee

Add running tasks metric and log running tasks for more than 24hrs wi…

8ed3e37

…th no updates

MatMaul requested review from clokep and erikjohnston August 14, 2023 10:43

Merge remote-tracking branch 'origin/develop' into mv/task-scheduler

60ea494

erikjohnston reviewed Aug 16, 2023

View reviewed changes

Mathieu Velten added 4 commits August 16, 2023 16:18

Remove unused sql file

c089f67

Address reviews

d158b67

Address comment

223f87e

Fix

e5a5344

MatMaul requested a review from erikjohnston August 16, 2023 15:13

erikjohnston approved these changes Aug 17, 2023

View reviewed changes

synapse/storage/databases/main/task_scheduler.py Outdated Show resolved Hide resolved

MatMaul requested a review from erikjohnston August 18, 2023 09:03

Address comment

56ab35e

MatMaul merged commit 358896e into develop Aug 21, 2023
37 checks passed

MatMaul deleted the mv/task-scheduler branch August 21, 2023 12:17

erikjohnston added a commit that referenced this pull request Sep 1, 2023

Fix typo where we ended up with multiple WorkerLocksHandler

3d4533b

I don't think has caused any actual issues. Introduced in #15891

erikjohnston mentioned this pull request Sep 1, 2023

Fix typo where we ended up with multiple WorkerLocksHandler #16220

Merged

erikjohnston added a commit that referenced this pull request Sep 1, 2023

Fix typo where we ended up with multiple WorkerLocksHandler (#16220)

93f2fdd

I don't think has caused any actual issues. Introduced in #15891

DMRobertson pushed a commit that referenced this pull request Sep 1, 2023

Fix typo where we ended up with multiple WorkerLocksHandler (#16220)

b85c348

I don't think has caused any actual issues. Introduced in #15891

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implements a task scheduler for resumable potentially long running tasks #15891

Implements a task scheduler for resumable potentially long running tasks #15891

MatMaul commented Jul 7, 2023 •

edited

clokep commented Jul 12, 2023

clokep Jul 14, 2023

clokep commented Jul 14, 2023

MatMaul commented Jul 17, 2023

clokep commented Aug 8, 2023

erikjohnston left a comment

erikjohnston Aug 11, 2023

MatMaul Aug 11, 2023 •

edited

MatMaul commented Aug 11, 2023

MatMaul commented Aug 14, 2023

erikjohnston left a comment

erikjohnston left a comment

		@@ -0,0 +1 @@
		Implements a task scheduler for resumable potentially long running tasks.

Implements a task scheduler for resumable potentially long running tasks #15891

Implements a task scheduler for resumable potentially long running tasks #15891

Conversation

MatMaul commented Jul 7, 2023 • edited

Pull Request Checklist

clokep commented Jul 12, 2023

clokep Jul 14, 2023

Choose a reason for hiding this comment

clokep commented Jul 14, 2023

MatMaul commented Jul 17, 2023

clokep commented Aug 8, 2023

erikjohnston left a comment

Choose a reason for hiding this comment

erikjohnston Aug 11, 2023

Choose a reason for hiding this comment

MatMaul Aug 11, 2023 • edited

Choose a reason for hiding this comment

MatMaul commented Aug 11, 2023

MatMaul commented Aug 14, 2023

erikjohnston left a comment

Choose a reason for hiding this comment

erikjohnston left a comment

Choose a reason for hiding this comment

MatMaul commented Jul 7, 2023 •

edited

MatMaul Aug 11, 2023 •

edited