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 potential time shift in EveryJob #181

Merged
merged 1 commit into from Nov 28, 2015

Conversation

Projects
None yet
2 participants
@Korrigan
Contributor

Korrigan commented Nov 27, 2015

Hi,

First I'd like to thank you for this awesome work on rufus-scheduler :)

I use rufus-scheduler (with resque-scheduler) in production to run jobs every 60s and I noticed that the schedule time shifted by ~1s every 15 minutes.

This was problematic to me because I rely heavily on time to do my computations, so I read the code and found the set_next_time method of Rufus::Scheduler::EveryJob was relying on the trigger time to define the next run of the job. I implemented a change to rely on the old @next_time value if it is defined to avoid time shift due to compute time or tick interval, it seems more logical and actually works perfectly for me.

What do you think ?

Please let me know if anything is unclear

Best regards,

fix potential time shift in EveryJob. We use the last @next_time valu…
…e and add @frequency instead of relying of the trigger time which may vary
@jmettraux

This comment has been minimized.

Show comment
Hide comment
@jmettraux

jmettraux Nov 27, 2015

Owner

Salut Matthieu, I'm ready to fix that, but a good first step would be a spec.

Owner

jmettraux commented Nov 27, 2015

Salut Matthieu, I'm ready to fix that, but a good first step would be a spec.

@jmettraux

This comment has been minimized.

Show comment
Hide comment
@jmettraux

jmettraux Nov 27, 2015

Owner

OK, I will keep this PR open and consider it an issue report. I will add a spec and work a fix.

Thanks.

Owner

jmettraux commented Nov 27, 2015

OK, I will keep this PR open and consider it an issue report. I will add a spec and work a fix.

Thanks.

@jmettraux

This comment has been minimized.

Show comment
Hide comment
@jmettraux

jmettraux Nov 28, 2015

Owner

Changed my mind, merging first.

Owner

jmettraux commented Nov 28, 2015

Changed my mind, merging first.

jmettraux added a commit that referenced this pull request Nov 28, 2015

@jmettraux jmettraux merged commit 56bae1f into jmettraux:master Nov 28, 2015

1 check failed

continuous-integration/travis-ci/pr The Travis CI build failed
Details

jmettraux added a commit that referenced this pull request Nov 28, 2015

add commented out spec for gh-181
runned it manually against previous (pre @Korrigan code) but couldn't make it fail (...!!! display)

[ci skip]

jmettraux added a commit that referenced this pull request Nov 29, 2015

jmettraux added a commit that referenced this pull request Nov 29, 2015

@jmettraux

This comment has been minimized.

Show comment
Hide comment
@jmettraux

jmettraux Nov 29, 2015

Owner

Merci beaucoup !

Owner

jmettraux commented Nov 29, 2015

Merci beaucoup !

@Korrigan

This comment has been minimized.

Show comment
Hide comment
@Korrigan

Korrigan Nov 30, 2015

Contributor

You're welcome !

Thank you for refining/testing my PR :)

Contributor

Korrigan commented Nov 30, 2015

You're welcome !

Thank you for refining/testing my PR :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment