-
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
Conversation
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.
Looking good. Left some thoughts.
bin/fuel-core/src/cli/rollback.rs
Outdated
@@ -30,7 +30,7 @@ pub struct Command { | |||
#[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() | |||
default_value = getrlimit(Resource::NOFILE).map(|(_, hard)| i32::try_from(hard.saturating_div(2)).unwrap_or(i32::MAX)).expect("Failed to get OS max file descriptors.").to_string() |
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.
Oh. Sorry. I mean specify why it shouldn't panic:
We recommend that expect messages are used to describe the reason you expect the Result should be Ok.
As suggested by the docs:
https://doc.rust-lang.org/std/result/enum.Result.html#method.expect
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.
Sorry I didn't correctly understood you first message and didn't know about this convention. Happy to learn :D Ty
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.
Oh didn't know this. I've definitely done a bunch of .expect("failed to blah blah")
😅 TIL
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.
Looks correct to me. I think this would be even better if we used an Option<u32>
instead of an i32
where -1
has a special meaning.
/// If defined as -1 no limit will be applied and will use the OS limits. | ||
/// If not defined the system default divided by two is used. |
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.
What's the reason for using -1
to mean no limit as opposed to take an Option<u32>
?
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.
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 comment
The reason will be displayed to describe this comment to others. Learn more.
And we want to have different behaviour between :
- No defined value which will be using OS divided by two
- and max value of the system which is -1
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.
Alright, fair enough.
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 agree that this limit should generally be Option<u32>
, but this can only represent two states, not the three we need. Let's keep it like that for the sake of simplicity. But we should maybe sanitize this value and not allow -2, -3, etc.?
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.
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.
bin/fuel-core/src/cli/rollback.rs
Outdated
#[clap( | ||
long = "rocksdb-max-fds", | ||
env, | ||
default_value = getrlimit(Resource::NOFILE).map(|(_, hard)| i32::try_from(hard.saturating_div(2)).unwrap_or(i32::MAX)).expect("Our supported platforms should return max FD.").to_string() |
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.
This is quite long to be in an attribute. How about breaking this out to a helper function?
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.
Good idea.
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
bin/fuel-core/src/cli/run.rs
Outdated
#[clap( | ||
long = "rocksdb-max-fds", | ||
env, | ||
default_value = getrlimit(Resource::NOFILE).map(|(_, hard)| i32::try_from(hard.saturating_div(2)).unwrap_or(i32::MAX)).expect("Our supported platforms should return max FD.").to_string() |
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.
Same here
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
bin/fuel-core/src/cli/snapshot.rs
Outdated
#[clap( | ||
long = "rocksdb-max-fds", | ||
env, | ||
default_value = getrlimit(Resource::NOFILE).map(|(_, hard)| i32::try_from(hard.saturating_div(2)).unwrap_or(i32::MAX)).expect("Our supported platforms should return max FD.").to_string() |
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.
And here
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
Ready to approve after those last changes are made. |
@@ -75,6 +75,7 @@ impl BenchDb { | |||
tmp_dir.path(), | |||
None, | |||
Default::default(), | |||
-1, |
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)
@@ -19,11 +23,28 @@ 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 comment
The 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 comment
The 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.
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.
Nice!
Linked Issues/PRs
Resolves: #2093
Description
Add a flag to define the maximum number of file descriptors that RocksDB can use. By default it's half of the OS limit.
Checklist
Before requesting review