Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

service: simplify Stop() & improve errors #884

Merged
merged 3 commits into from
Apr 17, 2019
Merged

service: simplify Stop() & improve errors #884

merged 3 commits into from
Apr 17, 2019

Conversation

ilgooz
Copy link
Contributor

@ilgooz ilgooz commented Apr 16, 2019

No description provided.

@ilgooz ilgooz requested a review from a team April 16, 2019 10:48
@ilgooz ilgooz force-pushed the refactoring/service/simplify-stop branch 5 times, most recently from 2fb5770 to 215bd2d Compare April 16, 2019 11:03
@ilgooz ilgooz added the wip label Apr 16, 2019
@ilgooz ilgooz force-pushed the refactoring/service/simplify-stop branch from 215bd2d to 9e8eb64 Compare April 16, 2019 13:05
@ilgooz
service: simplify Stop() & improve errors
c254b15 
* moved Dependency.Stop() logic into Service.Stop().
* return Stop() errors for all dependencies, not just the first one.
* removed unneeded TestStopDependency() & TestIntegrationStopDependency() tests.
@ilgooz ilgooz force-pushed the refactoring/service/simplify-stop branch from 9e8eb64 to c254b15 Compare April 16, 2019 13:19
@ilgooz ilgooz removed the wip label Apr 16, 2019
antho1404
antho1404 reviewed Apr 17, 2019
View reviewed changes
Copy link
Member

@antho1404 antho1404 left a comment

Choose a reason for hiding this comment

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

why did you remove the mutex here ? I'm not really sure why we had one in the first place maybe to fix some strange behaviors with docker when we stop containers or something like that

@ilgooz
Copy link
Contributor Author

ilgooz commented Apr 17, 2019

why did you remove the mutex here ? I'm not really sure why we had one in the first place maybe to fix some strange behaviors with docker when we stop containers or something like that

That mutex was there to prevent concurrent writes to err. But with the updated version, it's handled inside errs.Append().

@ilgooz ilgooz requested a review from NicolasMahe April 17, 2019 07:44
@ilgooz ilgooz merged commit 7e9a258 into dev Apr 17, 2019
@ilgooz ilgooz deleted the refactoring/service/simplify-stop branch April 17, 2019 10:03
ilgooz added a commit that referenced this pull request Apr 17, 2019
@ilgooz
service: fix stop order between service & dependencies
91b3509 
this bug was introduced by #884.

service itself should always be stopped first to give it a chance to properly shutdown itself. if depencies were stopped first, service may try to reconnect them or may start some fault toleration protocols for no reason.
@ilgooz ilgooz mentioned this pull request Apr 17, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@NicolasMahe NicolasMahe NicolasMahe approved these changes

@krhubert krhubert krhubert approved these changes

@antho1404 antho1404 antho1404 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@ilgooz @antho1404 @krhubert @NicolasMahe