implement @start job #52
Conversation
|
@krallin This would be a useful addition. Is anything missing to have this reviewed? |
There was a problem hiding this comment.
Thanks for contributing. I'm sorry this took so long to review — this hadn't really been on my radar at all :(
I left some suggestions inline. If you'd like to make those, let me know. Otherwise, considering how long we've kept you waiting, I'm happy to finish this up for you. Let me know.
Thanks!
| @@ -100,11 +100,18 @@ func ParseCrontab(reader io.Reader) (*Crontab, error) { | |||
| continue | |||
| } | |||
|
|
|||
| jobLine, err := parseJobLine(line) | |||
| // replace start with so the line will parse without errors | |||
| jobLine, err := parseJobLine(strings.Replace(line, StartExpressionValue, "0 0 0 1 1 * *", 1)) | |||
There was a problem hiding this comment.
Is there a reason to call parseJobLine if we're not really trying to parse a cron expression here? If the job line starts with @start, how about we just split that out and return a crontab line directly, as opposed to roundtrip through parseJobLine with dummy data?
| if expression == crontab.StartExpr { | ||
| fn(nextRun, logger) | ||
| return | ||
| } |
There was a problem hiding this comment.
I'd rather handle this purely within the expression interface. Right now, this feels a little hacky, and as a result it means start jobs will not get to finish if you CTRL+C (jobWg.Wait() won't wait for anything and wg.Done() will be called immediately). We also, get incomplete logging.
To do this cleanly, I think we need 2 changes to the interface for expression.Next():
- Give it a counter of how many times we have gone through this loop already (we ahve this variable in
cronIterationalready). - Return a pointer to a time, then we can check for null and break out of this loop if null.
| "job.command": job.Command, | ||
| "job.position": job.Position, | ||
| }) | ||
| if firstRun { |
There was a problem hiding this comment.
I'd rather keep the code a little simpler and re-run the start jobs if you reload the crontab.
There are 2 reasons for this:
- First, it keeps the code simpler. We don't need 2 blocks to run start jobs and regular jobs separately, etc.
- It enables the user to do more. With the approach you have here, you cannot run
@startjobs when reloading the crontab. With the approach I'm proposing, you can choose to not run them (remove them from the crontab before reloading), or to run them (don't remove them).
@start might be a bit un-ideal as a name then. In the issue folks were mentioning using @reboot. I think that'd work well here.
There was a problem hiding this comment.
Yea, I think restarting the cron jobs when container restarts is very reasonable. Could we get this into the main code? I really would like to use it and keep using the mainline code.
There was a problem hiding this comment.
i think it is not a good idea to run all @start jobs again on a reload. if i decleare a job with @start i want the job to run only once at container start.
if we want to be compatible with other cron scheduler we should use @reboot.
if the job should be executed every time the crontab is reloaded, @reboot is a bad name option. mabye @reload is a better option to make it clear, but I'm not happy with it either.
There was a problem hiding this comment.
Unlike the other review comments, it seems this needs a decision, on two points:
- Name. I agree that compatibility with
@rebootsounds like a good idea, rather than a custom@start. - Whether to run on reload or not. For compatibility with other cron systems, I agree it should not run on reload. This code could probably be simplified. There's no "on load" functionality in Vixie's
crontab, so I don't understand why the flexibility would be worth having (without a specific usecase.) That said, I store the crontab in container images, so I never use reload.
|
Can we get this merged? Any reason it's still pending? Looks like the project is getting updates and this solves the issue. |
|
currently I do not have time to implement the requested changes, maybe towards the end of the year |
|
@krallin if you would complete the pr I would be very happy |
|
At this point we do not have the bandwidth to carry this feature to the finish line. However, If someone would like to pick this back up we would happily review it. |
this pull request implements the issue #30
the start jobs will be executed first in parallel and must complete to start the regular jobs.