Skip to content
This repository has been archived by the owner on Nov 8, 2022. It is now read-only.

Fixed 1520: Cannot remove an Ended Task #1529

Merged
merged 1 commit into from
Mar 2, 2017

Conversation

IzabellaRaulin
Copy link
Contributor

@IzabellaRaulin IzabellaRaulin commented Feb 17, 2017

Fixed #1520

It's READY to review :)

To do:

  • Adding medium tests
  • Making an Ended state resumable

Summary of changes:

  1. Added documentation about Ended state in docs/TASKS.md

  2. Ended tasks can be removed
    See scheduler/task.go#L431)
    Before trying to remove Ended task:

    $ snaptel task remove 5db259f7-3403-421d-896a-78aa15031c80
    Error stopping task:
    Task must be stopped
    

    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 :

    $ snaptel task remove 71cf766a-7d95-48ed-ad74-f73853545476
    Task removed:
    ID: 71cf766a-7d95-48ed-ad74-f73853545476
    
    //verification (for this example, a task list should be empty now)
    $ snaptel task list
    No task found. Have you created a task?
    
  3. 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:

    $ snaptel task start       fc303c34-0484-4507-b0f8-7f7c526d3983
    Error starting task:
    error 0: Subscription already exists
    

    After trying to restart Ended task:
    (an approriate message will be returned if the schedule is not valid, what happens for windowed schedule)

    $ snaptel task start 71cf766a-7d95-48ed-ad74-f73853545476
    Error starting task:
    error 0: Task is ended. Cannot be restarted.
    

    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.

  4. 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

    $ snaptel task stop 71cf766a-7d95-48ed-ad74-f73853545476
    Error stopping task:
    error 0: Task is ended. Only running tasks can be stopped.
    

Diagram for Ended task

This diagram presents flow for Ended task (sum up all points above):

taskendedstatediagram

Testing done:

  • manually tested
  • medium tests

@intelsdi-x/snap-maintainers

@andrzej-k
Copy link
Contributor

@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?

@IzabellaRaulin IzabellaRaulin force-pushed the fixed_1520 branch 2 times, most recently from a351a59 to 6a896ff Compare February 20, 2017 19:38
@IzabellaRaulin IzabellaRaulin changed the title Fixed 1520 Fixed 1520: Cannot remove an Ended Task Feb 20, 2017
err := s.StartTask(tsk.ID())
So(err[0].Error(), ShouldResemble, "Task is disabled. Cannot be started.")
So(t.state, ShouldEqual, core.TaskDisabled)
})
Copy link
Contributor Author

@IzabellaRaulin IzabellaRaulin Feb 21, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI, Those tests' cases which have been removed from scheduler/scheduler_test.go are covered by added tests in scheduler/scheduler_medium_test.go in lines 478 and 424

@kjlyon
Copy link
Contributor

kjlyon commented Feb 21, 2017

Documentation and tests are very complete. 👍 LGTM

@IzabellaRaulin
Copy link
Contributor Author

@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?

@andrzej-k
Copy link
Contributor

LGTM - how about squashing to one commit ? :)

@jcooklin
Copy link
Collaborator

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.

@IzabellaRaulin
Copy link
Contributor Author

IzabellaRaulin commented Feb 24, 2017

@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.

@IzabellaRaulin
Copy link
Contributor Author

IzabellaRaulin commented Feb 27, 2017

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

@jcooklin
Copy link
Collaborator

jcooklin commented Mar 1, 2017

LGTM. 🚢 it.

@IzabellaRaulin IzabellaRaulin merged commit 30f6399 into intelsdi-x:master Mar 2, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cannot remove an Ended Task (windowed scheduler)
4 participants