-
Notifications
You must be signed in to change notification settings - Fork 294
Fixed 1520: Cannot remove an Ended Task #1529
Conversation
@IzabellaRaulin: Do you think that it would make sense to open an Issue for https://github.com/raintank/snap-plugin-collector-snapstats to include "Ended" state in exposed metrics? |
a351a59
to
6a896ff
Compare
6a896ff
to
d5b26b6
Compare
err := s.StartTask(tsk.ID()) | ||
So(err[0].Error(), ShouldResemble, "Task is disabled. Cannot be started.") | ||
So(t.state, ShouldEqual, core.TaskDisabled) | ||
}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Documentation and tests are very complete. 👍 LGTM |
@andrzej-k, I fully agree that all possible task's states should be reflected in metrics provided by snap-plugin-collector-snapstats. @intelsdi-x/snap-maintainers - is it ok to be merged? |
LGTM - how about squashing to one commit ? :) |
This PR has an impact on the work being planned related to a single run task as it relates to the "ended" state and whether it can be restarted. Currently only the windowed scheduler puts a task in the ended state and restarting a task with a windowed schedule doesn't make sense since it would cycle back to ended immediately (for stop time being passed). If another scheduler also puts a task into an ended state, which is being proposed, it may make sense for the task to be restartable. In summary, I'm not convinced that an ended state should not be restartable. |
@jcooklin, I will update this PR to make an Ended state resumable. There is no problem to do that. It will work for the windowed schedule too - trying to restart ended windowed schedule will return an error with message "Stop time is in the past" what is the way how it works already for invalid stop_timestamp (see schedule/schedule.go#L12. So, I think that sounds good. |
6e853e9
to
a7819e0
Compare
Squashed to one commit and made an Ended state resumable (if the schedule is still valid). The description of this pull request was updated too. Ready to be reviewed again. cc: @andrzej-k, @jcooklin |
LGTM. 🚢 it. |
Fixed #1520
It's READY to review :)
To do:
Summary of changes:
Added documentation about Ended state in docs/TASKS.md
Ended tasks can be removed
See scheduler/task.go#L431)
Before trying to remove Ended task:
Notice that stopping an Ended task did not work either (the state still was Ended, so it was a standpoint).
After removing Ended task directly works fine :
An Ended state migh be resumed if the schedule is still valid
In
startTask()
there is a check if the schedule is still valid at this point of time, see (scheduler/scheduler.go#L487).Before trying to restart Ended task:
After trying to restart Ended task:
(an approriate message will be returned if the schedule is not valid, what happens for windowed schedule)
Notice: if another scheduler also puts a task into an ended state, which is being proposed in work related with a single run task, it may make sense for the ended task to be restartable.
Added an appropriate message that tasks in Ended state cannot be stopped
Before: trying to stop an Ended task
Stopping an Ended task does not work - there was no message, however, state of task still was Ended
After: trying to stop an Ended task
An appropriate message will be returned
Diagram for Ended task
This diagram presents flow for Ended task (sum up all points above):
Testing done:
@intelsdi-x/snap-maintainers