eventhandler: reduce scope of task states ticker can submit #1178
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
adnxn
force-pushed
the
reduce-ticker-scope
branch
from
996e301
to
69ad0e5
Compare
8 tasks
adnxn
force-pushed
the
reduce-ticker-scope
branch
from
69ad0e5
to
954bd70
Compare
richardpen
reviewed
Jan 12, 2018
agent/eventhandler/task_handler.go
Outdated
| TaskARN: taskARN, | ||
| Status: task.GetKnownStatus(), | ||
| Task: task, | ||
| if task.GetKnownStatus() < api.TaskStopped { |
There was a problem hiding this comment.
Can you add modify/add a unit test to cover this?
aaithal
reviewed
Jan 12, 2018
agent/eventhandler/task_handler.go
Outdated
| TaskARN: taskARN, | ||
| Status: task.GetKnownStatus(), | ||
| Task: task, | ||
| if task.GetKnownStatus() < api.TaskStopped { |
There was a problem hiding this comment.
- There's a remote possibility that task's known status would change between lines 187 and 190. Can you assign it to a variable instead of getting it twice?
- Also, you can reduce the levels of indentation by doing something like:
taskKnownStatus := task.GetKnownStatus()
if taskKnownStatus >= api.TaskStopped {
continue
}
event := api.TaskStateChange {
...
}
...
There was a problem hiding this comment.
sounds good to me. fixed.
adnxn
force-pushed
the
reduce-ticker-scope
branch
2 times, most recently
from
8dc8b3d
to
3b9aebe
Compare
aaithal
approved these changes
Jan 12, 2018
agent/eventhandler/task_handler.go
Outdated
| @@ -184,6 +184,10 @@ func (handler *TaskHandler) taskStateChangesToSend() []api.TaskStateChange { | |||
| // safety mechanism) and add it to the list of task state changes | |||
| // that need to be sent to ECS | |||
| if task, ok := handler.state.TaskByArn(taskARN); ok { | |||
| knownStatus := task.GetKnownStatus() | |||
| if knownStatus >= api.TaskStopped { | |||
There was a problem hiding this comment.
minor: Can you also add a comment for why this is done?
Reduce scope of task states the eventhandler ticker can submit. This change only allows the ticker to submit task states that are `< api.TaskStopped`.
adnxn
force-pushed
the
reduce-ticker-scope
branch
from
3b9aebe
to
2bbb8f3
Compare
adnxn
added a commit
that referenced
this pull request
Jan 12, 2018
adnxn
added a commit
to adnxn/amazon-ecs-agent
that referenced
this pull request
Jan 15, 2018
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
This is the fix for missing container exit code for stopped tasks. This change reduces the scope of task states the eventhandler ticker can submit and only allows the ticker to submit task states that are
< api.TaskStopped.Implementation details
Testing
make release)go build -out amazon-ecs-agent.exe ./agent)make test) passgo test -timeout=25s ./agent/...) passmake run-integ-tests) pass.\scripts\run-integ-tests.ps1) passmake run-functional-tests) pass.\scripts\run-functional-tests.ps1) passNew tests cover the changes:
Description for the changelog
Licensing
This contribution is under the terms of the Apache 2.0 License:
yes