You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
This PR supersedes #939. I realized there's a dev branch that already has cronos merged. I went ahead and applied the same logic from the previous PR into the dev branch.
New overloads are added to RecurringJob.AddOrUpdate to allow for specifying a start and/or end date in UTC. Specifying these dates would queue new jobs only if the cron expression is matched and this run falls within the date bounds (inclusive).
Specifying start and or end date creates an allowable window for jobs to be queued. Null start implies queue immediately like existing recurring jobs. Null end implies queue forever.
Start and end dates are stored in the recurring job dictionary which, in sql server, end up in the Hash table. I haven't tested with Redis but I assume the serializer automatically handles it just like the it did for sql server.
I think there's no impact on current users since the two fields are optional. The job scheduler checks if start/end dates have a value. If not, the existing logic is used.
Questions:
Regarding @burningice2866's comment, adding explicit overloads for start and end dates results in 48 overloads for AddOrUpdate. Would it be better to create a new method (e.g. AddOrUpdateBounded) that uses optional params or is that many overloads OK?
Regarding @aidmsu's comment and my response (point 1), I have made it so the dates must already be in UTC. Is there a scenario where that wouldn't work?
Does the UI portion need to be a part of this PR or can/should it be a part of another?
@odinserj@aidmsu@burningice2866 Any thoughts/feedback? I know the codecov check failed but the fix for that depends on keeping the 48 overloads or moving to a different method name with optional params.
I personally like the idea of having a new RecurringDateRangeJob class used for scheduling recurring jobs with a start and end date. That way you don't need to pollute the existing RecurringJob class with tons of new overloads.
On a somewhat related note, the travis build failed and it looks like mono crashed right after running unit tests. Does that indicate a problem with the unit tests or an one-off issue with the build? Can the travis build be run again without having to push a new commit?
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
odinserj
force-pushed
the
dev
branch
2 times, most recently
from
This PR supersedes #939. I realized there's a dev branch that already has cronos merged. I went ahead and applied the same logic from the previous PR into the dev branch.
Questions: