-
Notifications
You must be signed in to change notification settings - Fork 505
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
fix: update sysinfo and fix fd leak on linux #4163
Conversation
50cea01
to
c29b008
Compare
let process_name = process.name().to_str(); | ||
|
||
if let Some(process_name) = process_name { | ||
if process_name.contains("fluvio") { |
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.
can you make fluvio
as constant?
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!
crates/fluvio-cluster/src/delete.rs
Outdated
let mut sys = System::new(); | ||
sys.refresh_processes(); // Only load what we need. | ||
for process in sys.processes_by_exact_name(name) { | ||
sys.refresh_processes(sysinfo::ProcessesToUpdate::All); // Only load what we need. |
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 seems like repeating. can you change into common util?
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.
Sure! I created a process
mod.
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.
LGTM. Couple of minor nits
02bde02
to
7367ddf
Compare
Update sysinfo to 0.31.4 and fix file descriptor leak on Linux.
sysinfo
leaks file descriptors on Linux if the limit is not set.Spu Before:
Spu After: