feat(#25): add job manager and jobs. use scheduledtask as templates #54
Conversation
|
@francescovallone I'm a bit at loss here. First of all, there are a lot of breaking changes, and it is not clear how, when and why would somebody use Note: breaking changes are fine, if (a) warranted, and (b) the semantic version reflects that. However, let's make the role of |
|
Hello @isoos, to be honest, I'm not sure what is intended as a breaking change in this case. This PR changes the internal behavior of the Cron object but not the external one since the JobManager is an internal class and is not intended to be used outside the Cron interface. Also, you can achieve the same thing with the current interfaces. I just removed responsibilities from the ScheduledTask giving them to the JobManager and the Job class. |
There was a problem hiding this comment.
I'm not entirely sure I understand the use case we are trying to solve with this PR. Note: my use case with the package is very simple, and I may not have encountered the problems that the new changes would solve. Maybe split it to multiple PRs that change one thing at a time?
| Schedule get schedule; | ||
|
|
||
| bool get isRunning; | ||
| Task get task; |
There was a problem hiding this comment.
Removing isRunning is a breaking change for clients of this package.
|
|
||
| @override | ||
| Future<void> cancel() async { | ||
| _closed = true; | ||
| _overrun = false; |
There was a problem hiding this comment.
I'm not sure: why are these removed?
There was a problem hiding this comment.
The _overrun is not used anymore since the ScheduledTask is just a template for the Job
There was a problem hiding this comment.
I'm more interested in the why: why is not needed to track it anymore? Related: what if the scheduler wants the next execution to wait until the previous completes its running? Shouldn't that be a configuration option and this way we don't break the runtime behavior for existing clients?
There was a problem hiding this comment.
I thought it wasn't needed anymore since in the proposed implementation there is not a way to wait for a job to finish. But you are totally right this is a breaking change and should be added an option to manage the jobs like before. We can, however, prevent the breaking change making the option enabled by default.
|
The problem is with concurrent tasks that do not end within the start of a new one. For example one task is scheduled to start every minute but the execution takes 2 minutes. In this case the second tasks will not start because the first one is still running |
Hello, I tried to solve the problem regarding the JobManager (#25).
The current implementation uses a
JobManagerthat manages the jobs for theScheduledTaskthis means that the ScheduledTasks are now a template object, used only to group the jobs and to define if a new job should start.This feat should also solve #49 since the JobManager, contrary to the ScheduledTask, can run more instances of the job.
Let me know if something is not clear or if I need to change something 😄