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: synchronize the reserve sampling process #3614

Merged
merged 13 commits into from Dec 7, 2022
Merged

Conversation

aloknerurkar
Copy link
Contributor

@aloknerurkar aloknerurkar commented Dec 2, 2022

Checklist

  • I have read the coding guide.
  • My change requires a documentation update, and I have done it.
  • I have added tests to cover my changes.
  • I have filled out the description and linked the related issues.

Description

In the localstore we usually guard parallel operations using the batchMu mutex. This is a huge bottleneck, but one that cannot be avoided at the moment. Due to the nature of the current localstore code, we need to have this exclusive lock to protect any changes to the underlying DB.

For the sampling, we never acquired this lock. But we should as there could be changes to the pullIndex during iteration which should not happen. Also, currently, we do not acquire this lock when we evict batches from the batchstore. Both these cases are now fixed.

With this change, if there is a sampling process ongoing and reserve evictions happen, we will cancel the sampler. This will cause the player to not participate in the lottery game for that round. This is considered better as if the player does participate and end up submitting a wrong sample, he might get frozen. Also, having the lock for the sampling process seems to be an overkill and would slow down the Put path causing bottlenecks.

Along with this, I have added metrics for the BatchEviction process which were missing.

Fixes #3611 hopefully.


This change is Reviewable

pkg/localstore/sampler.go Outdated Show resolved Hide resolved
pkg/localstore/store_lock.go Outdated Show resolved Hide resolved
pkg/postage/batchstore/reserve.go Outdated Show resolved Hide resolved
Copy link
Member

@janos janos left a comment

Choose a reason for hiding this comment

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

The approach looks pretty good with not so drastic changes. It should be good to go after linter is satisfied.

pkg/localstore/store_lock.go Outdated Show resolved Hide resolved
@aloknerurkar aloknerurkar merged commit 6b32e80 into master Dec 7, 2022
@aloknerurkar aloknerurkar deleted the samplerlock branch December 7, 2022 10:02
@aloknerurkar aloknerurkar added this to the 1.10.0 milestone Dec 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

disable reserve sampler during chunks evictions
5 participants