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 date alert jobs #15573

Merged
merged 3 commits into from
May 17, 2024

Conversation

ulferts
Copy link
Contributor

@ulferts ulferts commented May 15, 2024

Safeguards against having too many ScheduleDateAlertsNotificationsJob instances by applying GoodJob's concurrency limitations. Doing so leads to always only having max one such job executed at the same time and max one such job queued. That queued job might get automatically rescheduled by GoodJob if the currently active job is still busy.

The change also prevents multiple jobs covering the same time interval. Now, a job will only consider time zones which are between the cron_at value of the previous job + 15 minutes till the cron_at value of the currently executed job. The cron_at value is independent of when the job is actually run so it is safe against delays. It is also safe against cases where one or multiple jobs are missed to be scheduled.

There is more explanation within the comments next to the code


https://community.openproject.org/wp/55101

@ulferts ulferts marked this pull request as ready for review May 16, 2024 06:59
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.

Looks good. One thing that @cbliard brought to our attention is that the concurrency limits will retry forever on a exponential backoff.

Should we add a discard_on call for the concurrency checks after a couple (let's say 5) tries?

@ulferts
Copy link
Contributor Author

ulferts commented May 17, 2024

Looks good. One thing that @cbliard brought to our attention is that the concurrency limits will retry forever on a exponential backoff.

Should we add a discard_on call for the concurrency checks after a couple (let's say 5) tries?

I'll issue another PR that applies the same concurrency pattern to the ScheduleReminderMailsJob and with it will add a retry_on for both by which the jobs will be retried twice with a backoff of 5 minutes and then discarded. Because after 15 minutes, the next job can be scheduled and take over.

@ulferts ulferts merged commit 8c4676f into release/14.1 May 17, 2024
10 checks passed
@ulferts ulferts deleted the fix/limit_schedule_date_alert_jobs branch May 17, 2024 11:24
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.

3 participants