Fixes #1598 and #1601 - ability to stop tasks gracefully #1604
Conversation
96184c3 to
472e549
Compare
| time.Sleep(dur) | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
@rashmigottipati, I know that introducing time_to_wait in snap-plugin-collector-mock2 was really helpful for testing gracefully shutdown behavior during working on it. However, I have a doubt about landing this change on master - is it really needed? Does it provide some value - I mean is there any large test which runs mock2 collector and testing this behavior? If yes, that is fine - I won't insist on removing that.
There was a problem hiding this comment.
@IzabellaRaulin thanks for testing it. I agree with you - I don't think it's necessary to land this change on master. I added another test in scheduler_test.go which has timeToWait in collect method and an assertion on the total elapsed time. In my opinion, it would suffice to land this test on master.
There was a problem hiding this comment.
If you remove this don't forget to remove the example task that was addded too.
scheduler/scheduler_test.go
Outdated
| time.Sleep(50 * time.Millisecond) | ||
| So(t.State(), ShouldResemble, core.TaskStopped) | ||
| c.timeToWait = 0 | ||
| }) |
There was a problem hiding this comment.
@rashmigottipati , I have doubt about this test. I will try to propose something another instead of doing that in this way. I will let you know when I have sth.
|
Besides comments above, general code LGTM |
ade1e95 to
7c4a900
Compare
|
I deleted the mock test example and changed the medium test to listen on TaskStoppedEvent instead of using time.Sleep(). |
scheduler/scheduler_medium_test.go
Outdated
| select { | ||
| case <-lse.TaskStoppedEvents: | ||
| So(time.Since(startTime), ShouldBeGreaterThan, c.timeToWait) | ||
| //elapsed time should be greater than 500ms |
There was a problem hiding this comment.
(small suggestion) This comment in line 402 should be above the code in line 401
There was a problem hiding this comment.
Similar suggestions for the following lines 398 & 397, 391 & 390
There was a problem hiding this comment.
Fixed them.
bfbb8e2 to
b8e37bb
Compare
scheduler/scheduler_medium_test.go
Outdated
| @@ -54,6 +55,7 @@ func (m *mockMetricManager) CollectMetrics(string, map[string]map[string]string) | |||
| } | |||
|
|
|||
| func (m *mockMetricManager) PublishMetrics([]core.Metric, map[string]ctypes.ConfigValue, string, string, int) []error { | |||
| time.Sleep(m.timeToWait) | |||
There was a problem hiding this comment.
Even that is only for testing purpose, I suggest moving this time.Sleep() to CollectMetrics rather than having that in the last step in the workflow which is PublishMetrics. The another reason is that collection normally takes longer than publishing, what seems to be more accurate to reality.
There was a problem hiding this comment.
I intended to add this to CollectMetrics. Thanks for pointing it out. Fixed it.
b8e37bb to
6649afb
Compare
scheduler/scheduler_medium_test.go
Outdated
| // the last scheduled workflow execution should be allowed to finish | ||
| // so we expect that stopping the task is going to happen not early than 500ms (set by by timeToWait) | ||
| //the task should have fired once | ||
| So(t.HitCount(), ShouldEqual, 1) |
There was a problem hiding this comment.
Please move checking the hitCount into the case <-lse.TaskStoppedEvents after the checking that task is Stopped (so the HitCount for 100% sure does not change)
6649afb to
7545a6f
Compare
7545a6f to
6ab21ce
Compare
Fixes #1598 and #1601
Summary of changes:
Testing done:
@intelsdi-x/snap-maintainers