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

Adding additional cron support (#, L, W) #494

Closed
jcoutch opened this issue Jan 11, 2016 · 26 comments
Closed

Adding additional cron support (#, L, W) #494

jcoutch opened this issue Jan 11, 2016 · 26 comments

Comments

@jcoutch
Copy link

jcoutch commented Jan 11, 2016

This is more of a feature request than an issue. We needed Hangfire to support a few cron features that the current version didn't support (# - day of week field, L - last day of week/last day of month, W - closest weekday to specified day.)

After digging through NCrontab's code this weekend, I determined it wouldn't be easy to implement within the current codebase, so I re-wrote the parser and created a new project called NCrontab-Advanced.

I'm currently testing a branch of Hangfire with these changes in place. Just putting this issue here as a placeholder in case anyone else needs this functionality. As soon as I have enough unit tests added to NCrontab-Advanced that I feel confident it's ready for prime time, I'll initiate the pull request.

Repos:
Hangfire using NCrontab-Advanced
NCrontab-Advanced repo

@gandarez
Copy link

Watching repos. If you need help on this please feel free to ask me.

@odinserj
Copy link
Member

Nice work, @jcoutch! I would be happy to use this more powerful cron parser. But how stable it is?

@jcoutch
Copy link
Author

jcoutch commented Jan 13, 2016

@odinserj - Right now, it passes all of NCrontab's unit tests. I've been adding unit tests around the additional features, and working to improve the algorithm that finds the next instance. Right now, it increments the DateTime by 1 until it finds the next instance, which is quick for short intervals. If you want to try it out on a test Hangfire instance, I have it integrated into the one in the repo (might need to upgrade the Nuget package), or you can swap out NCrontab for NCrontab.Advanced in Hangfire.Core, and replace using NCrontab; with using NCrontab.Advanced;

@dmm1981
Copy link

dmm1981 commented Jan 13, 2016

great work @jcoutch . Very keen to start using the more advanced cron options.

@jcoutch
Copy link
Author

jcoutch commented Jan 14, 2016

Over the past two days, I managed to include a lot more unit tests to get me to the point of being able to refactor InternalGetNextOccurrence (without fear of breaking things.) 😄 I was able to implement a similar algorithm to the one in NCrontab for finding the next instance, and the library is faster now. Just released version 1.1.0 of NCrontab-Advanced on Nuget, and updated the reference in my Hangfire branch.

I'm going to start testing the new functionality with Hangfire hopefully over the next couple days (if my work schedule permits.) If others following this issue are able to test as well, that'd be great!

@odinserj
Copy link
Member

Okay guys, I would be happy to accept these changes as a pull request after testing. Any feedback from other users is highly appreciated!

@jcoutch, you've done a really huge contribution!

@odinserj odinserj added this to the 1.6.0 milestone Jan 14, 2016
@dotnetdan
Copy link

This is great! 👍

@odinserj How far off do you think 1.6 release is?

@odinserj
Copy link
Member

odinserj commented Apr 5, 2016

@jcoutch, any news on this? I'm thinking how to incorporate this feature into an upcoming 1.6.0 release. Since NCrontab.Advanced has not so many downloads, we can claim advanced CRON expression features as experimental and activate NCrontab.Advanced through a switch (to not to break things).

For example, we can add bool Advanced property to the RecurringJobOptions class, having both references on NCrontab and NCrontab.Advanced. After more testing, in Hangfire 2.0, we'll switch all recurring jobs to NCrontab.Advanced.

@jcoutch
Copy link
Author

jcoutch commented Apr 7, 2016

@odinserj - Sorry for not being more active on this. The project I wrote NCrontab.Advanced for wound up getting tabled until a future date, and haven't had time to do testing with Hangfire.

I like the idea of having the advanced cron features as opt-in until more people are using NCrontab.Advanced. What about having the advanced functionality as a web/app.config option so devs can just upgrade their Nuget package, and not have to make any code changes to switch between the two?

@odinserj
Copy link
Member

@jcoutch, another problem with daylight savings revealed in #567. When the clock is moved backward during the daylight saving, it results to a missed runs, because NCrontab uses regular increment for seconds/minutes/etc. This problem also relate to NCrontab.Advanced, because it doesn't use time zones too.

Maybe it's worth to implement the crontab parser that completely solves this problem by having the knowledge about time zones, and release it to NuGet. In this case, I'll switch the implementation to the new one starting from Hangfire 1.6.0 to forget about daylight saving problems.

I know you may be busy, so I'll think about the implementation and will let you know about the results here.

@jcoutch
Copy link
Author

jcoutch commented May 15, 2016

@odinserj - I have some time over the next couple days, and will look into adding timezone support. Thinking of defaulting to the system's timezone (which is what the current implementation does), and allowing for different timezones via an overload that takes a TimeZoneInfo instance.

@illegitimis
Copy link

Valiant effort with NCrontab.Advanced. I have updated NCrontab to 3.1 and doing some logic on the CrontabSchedule.Parse calls to distinguish between IncludingSeconds flag set or not. Much better to have all that in an assembly. However, I believe parser should be self sufficient, detecting 5,6 or 7 parts automatically, and correctly distinguishing between CronStringFormat.WithSeconds and CronStringFormat.WithYears. CronExpressionDescriptor does that. In this way, Hangfire won't have to be modified.

@gandarez
Copy link

@odinserj when do you plan to merge it to master?

@philga
Copy link

philga commented Dec 29, 2016

Can anyone provide an update on the status of Hangfire support for #, L, W in NCrontab expressions?

I am very impressed with Hangfire but feel I need to offer those scheduling options to the users of our system.

@jcoutch
Copy link
Author

jcoutch commented Dec 30, 2016

@philga - If you need to use it right now, you can pull down the Hangfire source code, and apply the changes referenced in this commit from my fork: Replaced NCrontab with NCrontab-Advanced

TL;DR - Remove the NCrontab Nuget package, add the NCrontab.Advanced package, and replace all lines that have using NCrontab; with using NCrontab.Advanced;

That's what I wound up doing for the project I'm working on.

@philga
Copy link

philga commented Dec 30, 2016

@jcoutch

Thanks so much for the work you have done and for your response to my question. I am glad to do the build you suggest.

One final question. Are there any significant Issues or limitations I should keep in mind with respect to NCrontab-Advanced?

@jcoutch
Copy link
Author

jcoutch commented Dec 30, 2016

@philga - Performance should be on par with NCrontab. The only time that might not be the case is with certain cron expressions that don't have a next occurrence, or one waaaay far in the future. In that situation, it may take a few seconds for calls to GetNextOccurence to return. Improving the increment logic is on my to-do list.

jcoutch/NCrontab-Advanced#7

@jcoutch
Copy link
Author

jcoutch commented Jan 30, 2017

If you want to start using NCrontab.Advanced now, I created a new job type, and published Hangfire.RecurringDateRange. This is a plugin for running jobs with a specified cron string, but within a specified date range (like running a job every 3rd Wednesday between 1/1/2017 - 3/31/2017.) You can pass null's for the start/end dates, and it'll behave like RecurringJob.

Note - this is not a drop-in replacement for RecurringJob. I purposefully created a new job type so it wouldn't conflict with the existing background processor, but run alongside it. This is just for those who want to experiment with NCrontab.Advanced, but don't want to deal with building the separate branch, or for those who need date range functionality too. :-)

@XtremeOwnageDotCom
Copy link

XtremeOwnageDotCom commented Mar 20, 2017

#801 is to attempt to accomplish the same thing.

Also, using NCrontab.Advanced would be much more elegant then my solution: http://pastebin.com/iZ1FmBKa

+1.

@shaycraft
Copy link

Yeah I didn't know you had already worked on this which is why I made #801 . Basically, I just configured it to use ncrontab-advanced, and added a bunch of function overloads that would allow you to specify the cron string format (while defaulting to the old style).

@XtremeOwnageDotCom
Copy link

@odinserj What is the status for this improvement? Its been sitting around here for over a year.

@odinserj
Copy link
Member

Working on it, we've rewritten Cron parsing from scratch, with time zones in mind, correct DST handling in all the corner cases, all the features supported, and made it very fast. Planning to use the new library in 1.7.0.

@XtremeOwnageDotCom
Copy link

Excellent!

Any timelines available at all? This functionality would be the one missing piece currently.

@philga
Copy link

philga commented Mar 24, 2017

@jcoutch,

I want to thank you for all of the help you provided me at the end of December. By following the step by step procedures you provided, I was able to integrate NCrontab.Advanced into Hangfire. The integration went very smoothly. We deployed the system about a month ago, and it seems to be working well. We have several hundred users taking advantage of the advanced scheduled features you wrote. We could not have used Hangfire without the advanced scheduling features you provided. Thanks so much!

@odinserj odinserj mentioned this issue Jul 6, 2017
29 tasks
@odinserj odinserj removed this from the Hangfire 1.6.0 milestone Mar 2, 2018
@odinserj odinserj added this to the Hangfire 1.7.0 milestone Mar 2, 2018
@coolhome
Copy link
Contributor

coolhome commented Dec 10, 2018

I believe this is resolved with PR #853. I believe the project switched to Chronos instead of NCrontab.

@anvaravind
Copy link

Cron Expression similar to 0 0 1,L * does not work, is there any plan to extend support ?

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

No branches or pull requests