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

Eliminate locking from postings lists #889

Merged
merged 1 commit into from
Sep 7, 2018

Conversation

prateek
Copy link
Collaborator

@prateek prateek commented Sep 6, 2018

All the segments/readers do the necessary locking external to the postings list, so we don't need the locking inside the postings lists on top of that.

@codecov
Copy link

codecov bot commented Sep 6, 2018

Codecov Report

Merging #889 into master will increase coverage by 22.79%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #889       +/-   ##
==========================================
+ Coverage    55.8%   78.6%   +22.79%     
==========================================
  Files         392     396        +4     
  Lines       33328   33479      +151     
==========================================
+ Hits        18599   26316     +7717     
+ Misses      13067    5363     -7704     
- Partials     1662    1800      +138
Flag Coverage Δ
#dbnode 81.38% <ø> (+11.74%) ⬆️
#m3ninx 71.93% <100%> (+50.3%) ⬆️
#query 69.25% <ø> (+60.91%) ⬆️

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 2cf04be...e599114. Read the comment docs.

Copy link
Collaborator

@jeromefroe jeromefroe left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -37,9 +36,8 @@ var (
errIteratorClosed = errors.New("iterator has been closed")
)

// postingsList wraps a Roaring Bitmap with a mutex for thread safety.
// postingsList wraps a Roaring Bitmap with the m3ninx pl API.
Copy link
Collaborator

Choose a reason for hiding this comment

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

worth calling out that it's not goroutine-safe?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sure, will do in a follow up change.

@prateek prateek merged commit 78df65a into master Sep 7, 2018
@prateek prateek deleted the prateek/m3ninx/postings-locks branch September 7, 2018 03:17
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.

2 participants