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

[Draft] [Scheduler] Added 'outdated' option to scheduler command #3771

Merged

Conversation

maelanleborgne
Copy link
Contributor

Description

This PR follows #3770 and adds an outdated option to the scheduler command.

This allows the scheduler to run jobs that should have been processed earlier but did not (use cases : server downtime, cron not running every minute).

Usage

php bin/grav scheduler --outdated

Advice wanted

First of all if you have another idea for the option name I'm open to discussion, I don't think outdated is really relevant.

Now the main concern :
While working on this feature I realized the Job creationTime is not stored in the config, but rather set when instantiating the job. The logic I had in mind involved checking if the job had been created later than the previous scheduled execution, to prevent running a job before its first scheduled run.
4bb4069#diff-d62f46dc32787f2a5ecbcca58aa4601fc942e8b293a6cfa7a9c6286475cbac02R212

I see a few options to work around this :

  • Removing this test. The downside is that running the scheduler with the 'outdated' option may result in running new jobs to early for their first run.
  • Storing the creationTime in the scheduler.yaml config. This would work for custom jobs, but the default jobs don't store their config there so I don't think it would be such a good idea.
  • Create a new scheduler state in addition to success and error ('new' for example) and for each newly created/modified jobs, add an entry to data/scheduler/status.yaml with state: new and last-run: previous-scheduled-time. This way we can keep track of every change in the scheduler configuration and make sure we don't run the jobs before the next occurrence.

IMO the third option would be the best to cover all cases, but I'd be happy to hear your thoughts about this :)

@rhukster
Copy link
Member

rhukster commented Nov 8, 2023

I agree, the 3rd option would probably be the best solution, seems the most robust approach.

@rhukster rhukster merged commit a71403f into getgrav:develop Jan 5, 2024
@rhukster
Copy link
Member

rhukster commented Jan 5, 2024

Thanks for this

@rhukster
Copy link
Member

rhukster commented Jan 5, 2024

@maelanleborgne i'm seeing tests fail:

Time: 00:01.018, Memory: 112.00 MB

There was 1 failure:

---------
1) JobTest: Is overdue | "hourly job created 1 hour ago and last run 30 mn ago"
 Test  tests/unit/Grav/Common/Scheduler/JobTest.php:testIsOverdue
Failed asserting that false matches expected true.
#1  /home/runner/work/grav/grav/tests/unit/Grav/Common/Scheduler/JobTest.php:14
#2  /home/runner/work/grav/grav/vendor/bin/codecept:119

FAILURES!
Tests: 219, Assertions: 1877, Failures: 1.
Error: Process completed with exit code 1.

@maelanleborgne
Copy link
Contributor Author

maelanleborgne commented Jan 5, 2024 via email

@rhukster
Copy link
Member

rhukster commented Jan 5, 2024

Weird, it's passing now.. was in the automated tests.. i couldn't replicate locally:

https://github.com/getgrav/grav/actions/runs/7421540097

Must of been a time related issue?

@rhukster
Copy link
Member

rhukster commented Jan 5, 2024

And appologies, i missed the DRAFT bit.. worked find in my quick local testing and tests ran fine...

I'll revert though.

rhukster added a commit that referenced this pull request Jan 5, 2024
This reverts commit a71403f.

# Conflicts:
#	tests/unit/Grav/Common/Scheduler/SchedulerTest.php
@rhukster
Copy link
Member

rhukster commented Jan 5, 2024

This has been reverted, but you will probably need to recreate the PR because i can't reopen it once merged. Applogies again.

@maelanleborgne
Copy link
Contributor Author

Weird, it's passing now.. was in the automated tests.. i couldn't replicate locally:

https://github.com/getgrav/grav/actions/runs/7421540097

Must of been a time related issue?

Yes I messed up the tests, will fix with the new implementation.
Sorry for the inconvenience, I'll make a new PR once it's fully operational.

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