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
feat: Expose wait_for_compact #841
feat: Expose wait_for_compact #841
Conversation
d1b82d6
to
e3b78c6
Compare
src/db_options.rs
Outdated
/// comments in DB::Close() for details. | ||
/// | ||
/// Default: false | ||
pub fn set_close_db(&mut self, v: bool) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this method shouldn't be exposed, or alternatively should be marked unsafe? It allows the underlying database to be closed without dropping the DB object, and RocksDB is pretty light on guarantees about continuing to use a DB after it was closed.
Regardless of the return status, the DB must be freed.
From the wiki:
Q: Is it safe to close RocksDB while another thread is issuing read, write or manual compaction requests?
A: No. The users of RocksDB need to make sure all functions have finished before they close RocksDB. You can speed up the waiting by calling CancelAllBackgroundWork().
Looking at the implementation, there is a closed_
flag in RocksDB but it is only checked in Close and in the destructor
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another safe alternative would be remove this and add a pub fn wait_for_compact_and_close(self, opts: &WaitForCompactOptions) -> Result<(), Error>
method that consumes the DB instance
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmmm, ok for the time being, I'm going to remove set_close_db as I don't need it. We can revisit if someone needs this option
10850d4
to
6d7fe89
Compare
No description provided.