Skip to content

Conversation

@mintdaniel42
Copy link
Contributor

@mintdaniel42 mintdaniel42 commented Aug 11, 2024

When just adding the frequency to the current time, due to the system load the execution time will shift by a few milliseconds every run. So after a few hundred runs the delay will no longer be f.ex. 5 minutes but 5 minutes and 10 seconds. This is now fixed by always calculating the time remaining

When just adding the frequency to the current time, due to the system load the execution time will shift by a few milliseconds every run. This is now fixed
@amanteaux
Copy link
Member

amanteaux commented Aug 21, 2024

Thank you for your contribution!

When using a fixed delay schedule, the shifting of a few milliseconds after each run is expected: https://github.com/Coreoz/Wisp/tree/master?tab=readme-ov-file#basics-schedules

To get around this, the existing solution to have a fixed frequency schedule is to use a cron expression: https://github.com/Coreoz/Wisp?tab=readme-ov-file#cron

I am not against creating a fixed delay schedule implementation though. If you are interested, can you update your PR with:

  • Add a new FixedFrequencySchedule implementation
  • Unit test only this implementation (no need to make an integration test with the whole scheduler)
  • Add a new static method in Schedules to easily use this implementation
  • Update the README.md file accordingly

@amanteaux amanteaux changed the title Make sure the fixed delay doesn't shift Add a fixed frequency schedule Aug 21, 2024
@mintdaniel42
Copy link
Contributor Author

Done 👍🏼

@amanteaux
Copy link
Member

Thank you!! And sorry for the merge delay!

@amanteaux amanteaux merged commit 7ec65e9 into Coreoz:master Aug 28, 2024
@mintdaniel42
Copy link
Contributor Author

No worries. I have to thank you for the merge 😃

@amanteaux amanteaux added this to the 2.5.0 milestone Aug 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants