-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
Add new flag for maximum file descriptors in rocksdb. #2386
base: master
Are you sure you want to change the base?
Changes from 2 commits
87a2240
ef6a1d0
929f0bd
09d4ddd
82eaa5f
c1bec23
9d41bd5
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -75,6 +75,7 @@ impl BenchDb { | |
tmp_dir.path(), | ||
None, | ||
Default::default(), | ||
-1, | ||
) | ||
.unwrap(); | ||
let db = Arc::new(db); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,6 +5,10 @@ use fuel_core::{ | |
combined_database::CombinedDatabase, | ||
state::historical_rocksdb::StateRewindPolicy, | ||
}; | ||
use rlimit::{ | ||
getrlimit, | ||
Resource, | ||
}; | ||
use std::path::PathBuf; | ||
|
||
/// Rollbacks the state of the blockchain to a specific block height. | ||
|
@@ -19,6 +23,17 @@ pub struct Command { | |
)] | ||
pub database_path: PathBuf, | ||
|
||
/// Defines a specific number of file descriptors that RocksDB can use. | ||
/// | ||
/// If defined as -1 no limit will be applied and will use the OS limits. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What is the behavior if they use -2 or any other negative value? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Didn't found anything about it in rocksdb source code in Rust or C, so I tried myself in a test and it seems to work without issues. Maybe this is the code : https://github.com/facebook/rocksdb/blob/8b38d4b4006ca9fd49432ccc16d9911919870dd2/db/db_impl/db_impl_open.cc#L57 I'm not sure that I fully understand this code but fore me if the value is -2 it will be clamped to 20. |
||
/// If not defined the system default divided by two is used. | ||
Comment on lines
+28
to
+29
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What's the reason for using There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It is the default value used by RocksDB for no limit that's why they are a i32. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. And we want to have different behaviour between :
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Alright, fair enough. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I agree that this limit should generally be There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. RocksDB doesn't seems to consider these values as error : #2386 (comment) But we can do. I don't think it's very useful because it doesn't creates any problem and the only reason to tryy a -2 is to tryy to make a problem. |
||
#[clap( | ||
long = "rocksdb-max-fds", | ||
env, | ||
default_value = getrlimit(Resource::NOFILE).map(|(_, hard)| i32::try_from(hard.saturating_div(2)).unwrap_or(i32::MAX)).unwrap().to_string() | ||
)] | ||
pub rocksdb_max_fds: i32, | ||
|
||
/// The path to the database. | ||
#[clap(long = "target-block-height")] | ||
pub target_block_height: u32, | ||
|
@@ -32,6 +47,7 @@ pub async fn exec(command: Command) -> anyhow::Result<()> { | |
path, | ||
64 * 1024 * 1024, | ||
StateRewindPolicy::RewindFullRange, | ||
command.rocksdb_max_fds, | ||
) | ||
.map_err(Into::<anyhow::Error>::into) | ||
.context(format!("failed to open combined database at path {path:?}"))?; | ||
|
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.
There are a few places that we're still using
-1
. Is that okay?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 have let in benches because benches didn't used the test value before and I don't want them to be slowed down by a new limit (maybe it don't change anything but there is no risk for benches I think)