-
Notifications
You must be signed in to change notification settings - Fork 454
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
Fix LRU deadlock + regression test and improve comments / logging #862
Conversation
richardartoul
commented
Aug 28, 2018
•
edited
Loading
edited
- Fix LRU deadlock that could occur when the events channel was filled up. Achieved this by making the series object call WiredList.Update() outside of a lock as well as adding the concept of a NonBlockingUpdate. Also added a regression test.
- Made LRU events channel size configurable for stress testing purposes.
- Improve "invariant violated" style logging so that metrics are emitted if wired list invariants are violated.
- Added a new "CloseIfFromDisk" method to the database block as an additional layer of protection.
Codecov Report
@@ Coverage Diff @@
## master #862 +/- ##
==========================================
- Coverage 78.43% 78.36% -0.07%
==========================================
Files 391 391
Lines 33222 33268 +46
==========================================
+ Hits 26058 26072 +14
- Misses 5372 5392 +20
- Partials 1792 1804 +12
Continue to review full report at Codecov.
|
72bb0f4
to
81ae4a8
Compare
src/dbnode/storage/series/series.go
Outdated
if shouldUnlock { | ||
s.Unlock() | ||
} | ||
}() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we refactor this to avoid multiple unlock calls? Perhaps something like:
var (
b block.DatabaseBlock
updateList *block.WiredList
)
s.Lock()
defer func() {
s.Unlock()
if b != nil && updateList != nil {
updateList.BlockingUpdate(b) // Update outside of lock
}
}()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh yeah thats better thanks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM