-
Notifications
You must be signed in to change notification settings - Fork 704
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
Support RocksDB 7.x BackupEngineOptions #700
Support RocksDB 7.x BackupEngineOptions #700
Conversation
@aleksuss, @stanislav-tkach, could you help to review this PR? |
cde4d66
to
5efce4b
Compare
@aleksuss, I've fixed a clippy warning. Could you approve the workflow run again? |
Signed-off-by: Wenbo Zhang <ethercflow@gmail.com>
5efce4b
to
1827aea
Compare
@aleksuss, could you take a look at the PR when you next have a chance? |
1827aea
to
5fee0c0
Compare
@aleksuss, @stanislav-tkach, could you help to merge this? |
5fee0c0
to
afff50d
Compare
@@ -217,27 +224,40 @@ impl BackupEngine { | |||
} | |||
|
|||
impl BackupEngineOptions { | |||
// | |||
} | |||
pub fn new<P: AsRef<Path>>(backup_dir: P) -> Result<Self, Error> { |
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.
Please, add some description to the public methods.
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.
Done. Please have another look.
526105d
to
72ce11d
Compare
The main objective of this PR is to support
rocksdb_backup_engine_options_t
(introduced in facebook/rocksdb@78aee6f) so that we can call the associatedrocksdb_backup_engine_options_set_max_background_operations
function and achieve higher parallelism for backup and restore operations (notably, the current parallelism is 1, meaning that rocksdb will only copy/checksum/hardlink/whatever 1 file at a time, which is insufficient for rocksdb to full utilize hardware on even modest machines).One side-effect of this PR is that the signature for BackupEngine::open changes from
open<P: AsRef<Path>>(opts: &BackupEngineOptions, path: P)
toopen(opts: &BackupEngineOptions, env: &Env)
. This may cause some consumer code to fail to compile, but it aligns better with the RocksDB-properBackupEngine::Open()
signature, so I believe it's a worthwhile change.Finally, I'm not well-versed in ffi or the related patterns (like
_outlive
), so please carefully review those bits in the PR.Thanks!
Matt