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

Rework searchers to execute on a single reader #883

Merged
merged 2 commits into from
Sep 6, 2018
Merged

Conversation

prateek
Copy link
Collaborator

@prateek prateek commented Sep 5, 2018

The m3db index is composed of time based index blocks, each index block is a complete index for a particular time range. Each index block is composed of segments (from writes and/or bootstraps). m3ninx's searcher implementation assumed these segments did not share document ID ranges (by ID, I mean postings.ID; not []byte). This guarantee isn't possible with m3db, because we accept writes before we bootstrap (or after, in the case of topology changes). This PR relaxes the restriction -- it makes the query searchers operate on a segment at a time, instead of all the segments at once. This will work out better for query optimisation too.

Fixes one of the issues in #865

@codecov
Copy link

codecov bot commented Sep 5, 2018

Codecov Report

Merging #883 into master will increase coverage by 22.85%.
The diff coverage is 86.66%.

Impacted file tree graph

@@             Coverage Diff             @@
##           master     #883       +/-   ##
===========================================
+ Coverage   55.58%   78.44%   +22.85%     
===========================================
  Files         392      393        +1     
  Lines       33387    33304       -83     
===========================================
+ Hits        18558    26124     +7566     
+ Misses      13164     5386     -7778     
- Partials     1665     1794      +129
Flag Coverage Δ
#dbnode 81.35% <ø> (+11.86%) ⬆️
#m3ninx 72.39% <86.66%> (+50.96%) ⬆️
#query 67.96% <ø> (+59.62%) ⬆️

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 a852262...b32f26b. Read the comment docs.

@prateek prateek force-pushed the prateek/repro-865 branch 3 times, most recently from a299435 to 6dea743 Compare September 5, 2018 20:13
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, makes the searcher code a lot simpler, I wonder if the fact that doc ID's can be in multiple segments will cause difficulties in other areas. Either way though this makes the searcher code better.

src/m3ninx/search/proptest/issue865_test.go Show resolved Hide resolved
@prateek prateek merged commit faee8d9 into master Sep 6, 2018
@prateek prateek deleted the prateek/repro-865 branch September 6, 2018 00:41
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