issue-654: allow setting a stopTime for job. #760
Conversation
job.go
Outdated
| @@ -60,6 +61,13 @@ func (j *internalJob) stop() { | |||
| j.cancel() | |||
| } | |||
|
|
|||
| func (j *internalJob) stopTimeReached() bool { | |||
There was a problem hiding this comment.
In order to support time mocking, this function should accept a now time.Time value. then, when it is called, we should pass in the schedulers clockwork time now value
executor.go
Outdated
| if j.stopTimeReached() { | ||
| return | ||
| } |
There was a problem hiding this comment.
In order to cover all of the job types, this check should happen inside the runJob() method of the executor right after it checks if the job or executor contexts are canceled
There was a problem hiding this comment.
In order to support time mocking, we probably need to move the clockwork object off of the scheduler struct and into the executor struct, so that the clockwork time now value can be passed in here
There was a problem hiding this comment.
I'll take care of moving the clock. I'm going to open a PR to do that.
job.go
Outdated
| // WithStopAt sets the option for stopping the job at | ||
| // a specific datetime. |
There was a problem hiding this comment.
| // WithStopAt sets the option for stopping the job at | |
| // a specific datetime. | |
| // WithStopAt sets the option for stopping the job from running | |
| // after the specified time. |
job.go
Outdated
| // WithStopDateTime sets the final data & time after which the job should stop | ||
| // This datetime must be in the future and should be after the startTime (if specified) |
There was a problem hiding this comment.
| // WithStopDateTime sets the final data & time after which the job should stop | |
| // This datetime must be in the future and should be after the startTime (if specified) | |
| // WithStopDateTime sets the final date & time after which the job should stop. | |
| // This must be in the future and should be after the startTime (if specified). | |
| // The job's final run may be at the stop time, but not after. |
scheduler.go
Outdated
| @@ -473,6 +473,10 @@ func (s *scheduler) selectStart() { | |||
| next = j.next(s.now()) | |||
| } | |||
|
|
|||
| if j.stopTimeReached() { | |||
There was a problem hiding this comment.
This check shouldn't need to happen here, but instead in the selectExecJobsOutForRescheduling function. since the executor is checking, if the stop time is reached, new jobs will be caught by that and then not rescheduled.
job.go
Outdated
| } | ||
|
|
||
| // StopAtOption defines options for stopping the job | ||
| type StopAtOption func(*internalJob) error |
There was a problem hiding this comment.
Once my PR is merged, could you pull it in and change the signature here to accept a time.Time as well, and pass that in to avoid using time.Now() for validation inside the WithStopDateTime func?
|
@JohnRoesler Sure I'll handle these later today. Thanks for reviewing and commenting. |
… timing of calling stopTimeReached.
|
Hi @JohnRoesler I've made some changes with regard to your comments and now you could check this PR again. Thank you. |
Higan
changed the title
What does this do?
This PR exposes a
stopTimewhen creating a job so that the job cannot be executed once thestopTimeis reached.Which issue(s) does this PR fix/relate to?
Resolves #654
List any changes that modify/break current functionality
Have you included tests for your changes?
Yes I created several test cases in
scheduler_test.goregarding job creation withstopTimespecified and also the expected behavior whenstopTimeis reached.Did you document any new/modified functionality?
example_test.goREADME.mdNotes