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

improve mutex locking when writing index in boltdb-shipper #5830

Merged

Conversation

sandeepsukhani
Copy link
Contributor

What this PR does / why we need it:
We currently take an exclusive lock for each index write to check whether desired index file exists which can cause contention. This PR improves the code by checking index file exists with a read lock and then takes a write lock to create if it does not exist.

@liguozhong
Copy link
Contributor

The code of this PR would be helpful. After the PR is merged, we will release boltdb-shipper again to observe the optimization effect.

@liguozhong
Copy link
Contributor

I am waiting for this PR to be merged, please help to speed up the merge of this PR.

@liguozhong
Copy link
Contributor

@sandeepsukhani Thanks for the quick response

@liguozhong
Copy link
Contributor

It would be great if this PR could be merged in the 2.5 release.

@liguozhong
Copy link
Contributor

@cyriltovena Please help to review this PR as soon as possible,thanks!

Copy link
Contributor

@dannykopping dannykopping left a comment

Choose a reason for hiding this comment

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

LGTM

@sandeepsukhani
Copy link
Contributor Author

cla stuck, will see if reopening PR fixes it.

@sandeepsukhani sandeepsukhani reopened this Apr 8, 2022
@sandeepsukhani sandeepsukhani merged commit 67ba342 into grafana:main Apr 8, 2022
ok bool
)

var err error
db, ok = lt.dbs[name]
if !ok {
Copy link
Contributor

Choose a reason for hiding this comment

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

I might invert this if so that the read-lock and write-lock cases look the same. Also this would outdent the remainder of the function which is more idiomatic Go.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, I merged the PR already. Will open a follow-up PR.

ok bool
)

var err error
Copy link
Contributor

Choose a reason for hiding this comment

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

This could be moved inside the scope where it is used.

sandeepsukhani added a commit to sandeepsukhani/loki that referenced this pull request Apr 8, 2022
owen-d pushed a commit that referenced this pull request Apr 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants