search

vsivsi / meteor-job-collection

A persistent and reactive job queue for Meteor, supporting distributed workers that can run anywhere.
https://atmospherejs.com/vsivsi/job-collection
Other
385 stars 68 forks source link

Support custom functions for scheduling repeated jobs #216

Open mitar opened 7 years ago

mitar commented 7 years ago

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

vsivsi commented 7 years ago

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

mitar commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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

vsivsi commented 7 years ago

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

kalepail commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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)