Add additional check if task already run for report scheduler #3900
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jpbruinsslot
commented
Nov 27, 2024
Comment on lines
90
to
113
|
|
||
| # When the schedule has no schedule, but a task is | ||
| # already executed we should not push the task again | ||
| task_db = None | ||
| try: | ||
| task_db = self.ctx.datastores.task_store.get_latest_task_by_hash(report_task.hash) | ||
| if task_db and schedule.schedule is None: | ||
| self.logger.debug( | ||
| "Schedule has no schedule, but task already executed", | ||
| schedule_id=schedule.id, | ||
| scheduler_id=self.scheduler_id, | ||
| organisation_id=self.organisation.id, | ||
| ) | ||
| continue | ||
| except storage.errors.StorageError as exc_db: | ||
| self.logger.error( | ||
| "Could not get latest task by hash %s", | ||
| report_task.hash, | ||
| scheduler_id=self.scheduler_id, | ||
| organisation_id=self.organisation.id, | ||
| exc_info=exc_db, | ||
| ) | ||
| continue | ||
|
|
There was a problem hiding this comment.
For now I've opted to check if there is already a task available for the schedule. I want to optimize this a bit more by having 2 separate queries (and combine the results to one) of schedules that have no cron expression and no tasks associated, and schedules that have a cron expression. Since for those two cases we need to create report task.
However since the PR #3840 is time-sensitive I've opted for this change atm. I'll make a ticket for this optimalisation and implement that so that it can be merged at a later time.
9 tasks
Checklist for QA:
What works:It works |
|
How does this handle 'reruns' that are use initiated? |
I've update the code to check if tasks have run for that particular schedule. Meaning if a user wants a rerun a new scheduled run should be created. |
|
underdarknl
approved these changes
Nov 28, 2024
jpbruinsslot
added a commit
that referenced
this pull request
Dec 2, 2024
* main: (64 commits) Bug fix: KAT-alogus parameter is now organization member instead of organization code (#3895) Remove sigrid workflows (#3920) Updated packages (#3898) Fix mula migrations Debian package (#3919) Adds loggers to report flow (#3872) Add additional check if task already run for report scheduler (#3900) Create separate finding for Microsoft RDP port (#3882) fix: 🐛 allow boefje completion with 404 (#3893) Feature/improve rename bulk modal (#3885) Update scheduler folder structure (#3883) Translations update from Hosted Weblate (#3870) Increase max number of PostgreSQL connections (#3889) Fix for task id as valid UUID (#3744) Add `auto_calculate_deadline` attribute to Scheduler (#3869) Ignore specific url parameters when following location headers (#3856) Let mailserver inherit l1 (#3704) Change plugins enabling in report flow to checkboxes (#3747) Fix rocky katalogus tests and delete unused fixtures (#3884) Enable/disable scheduled reports (#3871) optimize locking in katalogus.py, reuse available data (#3752) ...
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Changes
Since we're allowing scheduled tasks that have a single recurrence. We need to check if the task is already on the queue and we need to check if the task has been executed before when there isn't a cron expression is specified. This PR addsthis additional check if task already run for the report scheduler.
QA notes
With #3840 check if you can schedule different type of reporting jobs with different recurrence and make sure that it executes tasks as expected.
Code Checklist
.envchanges files if required and changed the.env-distaccordingly.Checklist for code reviewers:
Copy-paste the checklist from the docs/source/templates folder into your comment.
Checklist for QA:
Copy-paste the checklist from the docs/source/templates folder into your comment.