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

Fix data race in FST Segment Reads #938

Merged
merged 1 commit into from
Sep 25, 2018

Conversation

prateek
Copy link
Collaborator

@prateek prateek commented Sep 25, 2018

H/T @sboschman for debugging and suggesting the issue.

Example failure of added test without fix - https://pastebin.com/mtBbNUr6

@codecov
Copy link

codecov bot commented Sep 25, 2018

Codecov Report

Merging #938 into master will decrease coverage by 0.02%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #938      +/-   ##
==========================================
- Coverage   77.98%   77.96%   -0.03%     
==========================================
  Files         411      411              
  Lines       34407    34406       -1     
==========================================
- Hits        26834    26823      -11     
- Misses       5730     5737       +7     
- Partials     1843     1846       +3
Flag Coverage Δ
#dbnode 81.52% <ø> (-0.05%) ⬇️
#m3ninx 75.21% <100%> (-0.01%) ⬇️
#query 64.31% <ø> (+0.01%) ⬆️
#x 84.72% <ø> (ø) ⬆️

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 afe161b...ef6aba7. Read the comment docs.

@prateek prateek merged commit eef049a into master Sep 25, 2018
@prateek prateek deleted the prateek/m3ninx/concurrent-queries-fst branch September 25, 2018 03:48
)
wg.Add(2)

for i := 0; i < 2; i++ {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is it worth adding some more parallelism to more likely hit the edge case?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Nah, even this version with 2 routines fails pretty consistently within 1-2 of the sub-tests running (of the 100 prop test runs per test execution). No point adding more, it'll only serve to slow down CI

errLock sync.Mutex
matchErr error
)
wg.Add(2)
Copy link
Collaborator

Choose a reason for hiding this comment

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

super nit: Perhaps put a number up front which you can pass to both wg.Add(...) and the for loop so that they always match (easier to turn up/down parallelism).

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, opened PR #940

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