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 scheduler for scan tasks #68

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

Add scheduler for scan tasks #68

wants to merge 1 commit into from

Conversation

marcelldls
Copy link
Contributor

@marcelldls marcelldls commented Oct 29, 2024

Why:

  • Set intervals rather than delays (avoid drift, add precision) currently the property name period is a bit disingenuous
  • Behavior for scan tasks that take longer than their interval. Now sets to skip - Get a useful warning message: Execution of job "readback_position (trigger: interval[0:00:00.010000], next run at: 2024-10-29 15:13:39 GMT)" skipped: maximum number of running instances reached (1)

Possibly useful options not used:

  • Dynamically add/remove tasks (Destroy tasks on exit?)
  • Pause tasks (during reconnect?)
  • Custom triggers (e.g. mock trigger so that we can step in testing, Cron type triggers)
  • Grace period

Uncertain:

  • Handle the log messages produced

@marcelldls marcelldls requested a review from GDYendell October 29, 2024 16:33
Copy link

codecov bot commented Oct 29, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 82.21%. Comparing base (bdab4fa) to head (edd8d49).

Additional details and impacted files
@@            Coverage Diff             @@
##             main      #68      +/-   ##
==========================================
- Coverage   82.75%   82.21%   -0.55%     
==========================================
  Files          23       23              
  Lines         928      911      -17     
==========================================
- Hits          768      749      -19     
- Misses        160      162       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@GDYendell
Copy link
Contributor

This seems good! I agree with both of your "Why"s.

I am not familiar with apscheduler. Is that considered the standard solution for this? Are there alternatives we should look at?

@marcelldls
Copy link
Contributor Author

Reasons for:

  • Support for asyncio
  • Actively maintained, 6300 stars, 300000 daily downloads (Seems to be usually used as a plugin in web applications django-apscheduler, Flask-APScheduler, etc.)
  • It seems to be popular enough that there are plenty of tutorials available https://www.youtube.com/watch?v=sRnsR-T0Lxc

Possible doubts:

  • Maybe too heavy a library for what we need?
  • Overhead on polling performance?

An alternative could be:

@marcelldls marcelldls mentioned this pull request Jan 22, 2025
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.

2 participants