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

Support custom functions for scheduling repeated jobs #216

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

mitar
Copy link
Collaborator

@mitar mitar commented Feb 19, 2017

Provides a way to hook into computing after when scheduling repeated jobs. It could be used to for example configure timezones.

@vsivsi
Copy link
Owner

vsivsi commented Feb 21, 2017

@mitar, thanks for this, but is this sufficient to handle all of the cases with repeating jobs etc?

@mitar
Copy link
Collaborator Author

mitar commented Feb 21, 2017

I have not used it yet, but I think it should. Do you see any reason why not?

But yes, I have not yet tested it by trying to make schedule with timezone support. But is there any other place where later.js is used? I was thinking of simply allowing one to use your own function instead of later.js and that this should be enough.

@vsivsi
Copy link
Owner

vsivsi commented Feb 21, 2017

Yes, Later.js gets used in job.done() when preparing the next run of a repeating job:

https://github.com/vsivsi/meteor-job-collection/blob/master/src/shared.coffee#L1059

@mitar
Copy link
Collaborator Author

mitar commented Feb 22, 2017

I completely missed that. Now I am not even sure if #218 is a complete fix.

@vsivsi
Copy link
Owner

vsivsi commented Feb 22, 2017

It should be, right? Because that bug only affected the first run of a repeating schedule...

@kalepail
Copy link

Once you guys get this sorted I'd love a ducumented example of how to implement for timezones and DST scenarios on repeating jobs. Have been struggling ever since DST started. 😕

@mitar Would be happy to test.

@mitar
Copy link
Collaborator Author

mitar commented Mar 19, 2017

I pushed new commits to finalize my proposal of the API.

@tyvdh, it would be great if you could try it out and implement functions to handle timezone scheduling. You have to implement two methods scheduleRepeatFirst and scheduleRepeatNext. I suggest you base it on this gist.

Moreover, I suggest that you extend repeatWait value on the doc with timezone field (which would then be added to the existing schedules and exceptions fields from later.js). Probably you will have to implement scrub method as well, to remove that extra field, so that this package does not complain that schema does not match.

@vsivsi
Copy link
Owner

vsivsi commented Mar 20, 2017

HI, thanks for doing this!

I think there should probably be one or more tests added (preferably that don't depend on momentTZ) that use these new function hooks (plus scrub) so that it will be more difficult to accidentally break this functionality in the future.

Thanks!

@vsivsi
Copy link
Owner

vsivsi commented Apr 23, 2017

I'm not integrating this into 1.5.0, releasing today, because there is still insufficient test coverage of this new functionality. I'll consider it for a future release once that is complete.

@tcastelli
Copy link

tcastelli commented May 24, 2017

Haven't seen this before mentioning you on the other issue @mitar
But you might be right about #218 not being a complete fix for the problem since when job.done is called the date can be set incorrectly as showed in #235 .

(If the time when you are processing the job is on the same range of the later.schedule when you do a job.done, the next calculated value can be set incorrectly)

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.

4 participants