Skip to content

add options for boltdb_index_client to avoid dead loop #1471

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

Merged
merged 1 commit into from
Jun 27, 2019

Conversation

mizeng
Copy link
Contributor

@mizeng mizeng commented Jun 21, 2019

As part of solution for grafana/loki#685, the purpose is to avoid opening boltdb fall into dead loop.

@bboreham
Copy link
Contributor

It seems that your issue is about accessing Boltdb from more than one process.
The Boltdb README says: "multiple processes cannot open the same database at the same time".

So I am not clear why a timeout will help.

@mizeng
Copy link
Contributor Author

mizeng commented Jun 24, 2019

@bboreham The first failing is like you said, But what I'm proposed is to solve the next more incoming requests hang issue.
If one request in the process hang on BoltDB for any reason, it should not block any incoming requests rather than keep hanging. Customers need to get a fast failed instead of hanging. I think that's why this boltdb client has timeout parameter.
Using default parameter anyway looks like improper for fast failed.

Quote from the boltdb description in code for this parameter:

// Timeout is the amount of time to wait to obtain a file lock.
// When set to zero it will wait indefinitely. This option is only
// available on Darwin and Linux.

Also Boltdb README says: "To prevent an indefinite wait you can pass a timeout option to the Open() function:".

@bboreham
Copy link
Contributor

So your aim is to fail every request from then on?

@mizeng
Copy link
Contributor Author

mizeng commented Jun 24, 2019

yes. Fast fail looks better than indefinite wait. What do you think?

@mizeng mizeng force-pushed the master branch 2 times, most recently from b4fd8b8 to caf4c39 Compare June 26, 2019 06:52
Copy link
Contributor

@bboreham bboreham left a comment

Choose a reason for hiding this comment

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

Now I understand the intention, I don't think this value is worth adding as an option - just hard-code it and put a comment explaining the expected outcome.

@mizeng
Copy link
Contributor Author

mizeng commented Jun 27, 2019

fair enough.

updated. could you plz help review and merge again?

@csmarchbanks csmarchbanks merged commit e6df7ed into cortexproject:master Jun 27, 2019
@csmarchbanks
Copy link
Contributor

Thanks!

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