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

Add regression test for and fix deadlock. #2169

Merged
merged 2 commits into from
Feb 26, 2020
Merged

Conversation

notbdu
Copy link
Contributor

@notbdu notbdu commented Feb 24, 2020

What this PR does / why we need it:

Fixes #2155 .

Special notes for your reviewer:

Does this PR introduce a user-facing and/or backwards incompatible change?:

NONE

Does this PR require updating code package or user-facing documentation?:

NONE

@notbdu notbdu force-pushed the bdu/leasemgr-deadlock branch from c82281b to 67b0c6b Compare February 24, 2020 22:46
@codecov
Copy link

codecov bot commented Feb 24, 2020

Codecov Report

Merging #2169 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@          Coverage Diff           @@
##           master   #2169   +/-   ##
======================================
  Coverage    72.2%   72.2%           
======================================
  Files        1016    1016           
  Lines       87985   87985           
======================================
  Hits        63597   63597           
  Misses      20122   20122           
  Partials     4266    4266
Flag Coverage Δ
#aggregator 82% <0%> (ø) ⬆️
#cluster 85.3% <0%> (ø) ⬆️
#collector 82.8% <0%> (ø) ⬆️
#dbnode 79.1% <0%> (ø) ⬆️
#m3em 74.4% <0%> (ø) ⬆️
#m3ninx 74.2% <0%> (ø) ⬆️
#m3nsch 51.1% <0%> (ø) ⬆️
#metrics 17.6% <0%> (ø) ⬆️
#msg 74.8% <0%> (ø) ⬆️
#query 68.3% <0%> (ø) ⬆️
#x 83% <0%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 67b0c6b...ae5b1a2. Read the comment docs.

}
wg.Add(2)
go func() {
wg.Done()
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should these be Done after the OpenLease call?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We just want to wait for the goroutine to bootstrap. The calls are done in a loop till the server is called. They were the original source of the deadlock.

Copy link
Collaborator

@arnikola arnikola left a comment

Choose a reason for hiding this comment

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

LGTM, small nit/question in test

@notbdu notbdu merged commit 05064b4 into master Feb 26, 2020
@justinjc justinjc deleted the bdu/leasemgr-deadlock branch May 14, 2020 16:14
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.

Adding new m3db namespace causes partial cluster OOM
2 participants