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.

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

[17.12] [manager/dispatcher] Synchronize Dispatcher.Stop() with incoming rpcs. #2514

Closed
wants to merge 3 commits into from

Conversation

thaJeztah
Copy link
Member

backport of #2495 for 17.12

Also included #2486 to get a clean cherry-pick

@thaJeztah
Copy link
Member Author

ping @anshulpundir PTAL: let me know if it's ok to include #2486 as well

@anshulpundir
Copy link
Contributor

Its OK to include #2486, but you must also include #2492 @thaJeztah

@codecov
Copy link

codecov bot commented Feb 13, 2018

Codecov Report

Merging #2514 into bump_v17.12 will decrease coverage by 5.27%.
The diff coverage is 78.18%.

@@               Coverage Diff               @@
##           bump_v17.12    #2514      +/-   ##
===============================================
- Coverage        66.92%   61.64%   -5.28%     
===============================================
  Files               76      129      +53     
  Lines            11158    21263   +10105     
===============================================
+ Hits              7467    13108    +5641     
- Misses            2929     6751    +3822     
- Partials           762     1404     +642

Signed-off-by: Anshul Pundir <anshul.pundir@docker.com>
(cherry picked from commit 6fa4dda)
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Signed-off-by: Anshul Pundir <anshul.pundir@docker.com>
(cherry picked from commit 7d507f6)
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Signed-off-by: Anshul Pundir <anshul.pundir@docker.com>
(cherry picked from commit 0b2778a)
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
@thaJeztah
Copy link
Member Author

Added #2492 👍

@thaJeztah
Copy link
Member Author

Looks like additional changes may be needed: #2495 (comment)

@thaJeztah
Copy link
Member Author

Update on the additional changes needed; moby/moby#36274 (review)

@nishanttotla and I discussed the data race failure in unit-tests and it should be OK to move forward. Because of how the waitgroup is used, the race should not have any undesired side-effects.

@anshulpundir @nishanttotla should this be ok to merge?

@thaJeztah
Copy link
Member Author

Looks like #2495 is being reverted in #2518

@andrewhsu
Copy link
Member

This PR should be closed because #2495 is not the best way to fix the thing. The better fix is in #2519

@thaJeztah
Copy link
Member Author

Do we still need #2492 ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants