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

Fix/limit schedule reminder mail jobs #15602

Merged
merged 2 commits into from
May 17, 2024

Conversation

ulferts
Copy link
Contributor

@ulferts ulferts commented May 17, 2024

Applies the same pattern as done for the ScheduleDateAlertsNotificationsJob in #15573 to the ScheduleReminderMailsJob. It now also takes the time between the two cron_at values to be the boundaries in which the job operates.

The similarities of the two jobs are now extracted into a shared module.

In order to avoid a single job being retried indefinitely a retry_on limit is added

@ulferts ulferts requested a review from mereghost May 17, 2024 11:31
@ulferts
Copy link
Contributor Author

ulferts commented May 17, 2024

@mereghost , this is the PR that will add the limits you asked for in #15573. I tested the behaviour locally and it seems to work as expected. The job is retried twice (with a 5 minute backoff) and then discarded. 5 minutes after that, the next job is scheduled according to the cron tab.

@ulferts ulferts force-pushed the fix/limit_schedule_reminder_mail_jobs branch from f978191 to 4335f30 Compare May 17, 2024 12:25
Copy link
Contributor

@mereghost mereghost left a comment

Choose a reason for hiding this comment

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

Nice. I'll apply the same patterns on other jobs that have the concurrency controls set.

@ulferts ulferts marked this pull request as ready for review May 17, 2024 12:39
@ulferts ulferts merged commit 071f005 into release/14.1 May 17, 2024
9 checks passed
@ulferts ulferts deleted the fix/limit_schedule_reminder_mail_jobs branch May 17, 2024 12:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants