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/cron jobs run more than once #455

Closed

Conversation

muk-ai
Copy link

@muk-ai muk-ai commented Oct 21, 2023

@marcelolx
Thank you for maintaining a great gem.

I made this PR with reference to your PR. (#396)
In your idea you were trying to calculate the scheduled time for cron, but I noticed that the jobs in rufus-scheduler already have that information. That is the previous_time.
I rewrote it to use previous_time for cron jobs and added a test. I would be happy to have it merged if there is no problem.

@marcelolx
Copy link
Member

@muk-ai I am not sure this solves the issue, we already use the previous_time PR as well https://github.com/sidekiq-scheduler/sidekiq-scheduler/pull/396/files#diff-0ddd2bcc9bd29aac261acdd76530920c85887c269cc29e3f054e465b4ac8699eR355, but don't rely solely on it...

I added this to my TODO list and will take a better look when I have more time and see if just this solves the problem or not. Thanks for opening the PR, though. Any help is always welcome!

@muk-ai
Copy link
Author

muk-ai commented Oct 25, 2023

Oops, you were already using previous_time and trying to take some further care of it.
I will wait for your PR. You can refer to my test code if you like.

@muk-ai muk-ai closed this Oct 25, 2023
@muk-ai muk-ai reopened this Dec 3, 2023
@muk-ai
Copy link
Author

muk-ai commented Dec 3, 2023

@marcelolx
Sorry, I did not understand how busy you were.

Let me explain about my motivation.
To completely prevent the sidekiq scheduler from running multiple cron jobs, the server must be configured as follows.
image

I would like to do that as follows.
image

There are three advantages to doing this.

  • The scheduler becomes redundant.
  • No longer have to run two differently configured sidekiqs.
  • It prevents the users from unknowingly having multiple job runs.

So I would like you to check my PR again when you have the time, of course.
If you are unsure whether it will work well, you can make it an optional experimental feature. Then you can release it and we can try it out to see if it works in our production.

@afrase afrase mentioned this pull request Mar 7, 2024
@marcelolx
Copy link
Member

Thanks @muk-ai! Would you have some time to look at #463? I see you just used previous_time, but that might not work if Rufus runs the jobs precisely at the time it is supposed to run — i.e. run every minute, if it runs exactly at 12:00:00 we would return 11:59:00 even though it could have already run previously at this time

Let me know what you think (feel free to continue the discussion in #463) and then we can work together to get this finally in the library 🙏

@marcelolx
Copy link
Member

Closing in favor of #463

@marcelolx marcelolx closed this May 16, 2024
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.

None yet

2 participants