-
Notifications
You must be signed in to change notification settings - Fork 3.7k
[Enhancement](metrics) Avoid update_process_fd_num reporting no such file error indiscriminately. #26013
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
[Enhancement](metrics) Avoid update_process_fd_num reporting no such file error indiscriminately. #26013
Conversation
|
run buildall |
|
clang-tidy review says "All clean, LGTM! 👍" |
|
(From new machine)TeamCity pipeline, clickbench performance test result: |
…e error indiscriminately.
1c29115 to
0836acb
Compare
|
run buildall |
morningman
left a comment
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
|
PR approved by at least one committer and no changes requested. |
|
PR approved by anyone and no changes requested. |
|
clang-tidy review says "All clean, LGTM! 👍" |
|
(From new machine)TeamCity pipeline, clickbench performance test result: |
be/src/util/doris_metrics.cpp
Outdated
|
|
||
| process_fd_num_used->set_value(count); | ||
|
|
||
| auto limits = fmt::format("/proc/{}/limits", pid); |
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 use ‘/proc/self/limits’
be/src/util/doris_metrics.cpp
Outdated
| process_fd_num_used->set_value(0); | ||
| return; | ||
| } | ||
| for (const auto& entry : dict_iter) { |
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.
use count_if to replace this for loop
be/src/util/doris_metrics.cpp
Outdated
| if (!st.ok()) { | ||
| LOG(WARNING) << "failed to count fd: " << st; | ||
| std::error_code ec; | ||
| std::filesystem::directory_iterator dict_iter(fmt::format("/proc/{}/fd/", pid), ec); |
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.
use ‘/proc/self/fd/'
|
clang-tidy review says "All clean, LGTM! 👍" |
|
run buildall |
be/src/util/doris_metrics.cpp
Outdated
| void DorisMetrics::_update_process_thread_num() { | ||
| int64_t pid = getpid(); | ||
| std::stringstream ss; | ||
| ss << "/proc/" << pid << "/task/"; |
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.
remove this dead code
be/src/util/doris_metrics.cpp
Outdated
| return; | ||
| } | ||
| int64_t count = std::count_if(dict_iter, fs::directory_iterator(), | ||
| [](const fs::directory_entry& entry) { |
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.
in lambda , use const auto & entry to replace const fs::directory_entry& entry
5b2bb09 to
3fd5472
Compare
|
clang-tidy review says "All clean, LGTM! 👍" |
1 similar comment
|
clang-tidy review says "All clean, LGTM! 👍" |
|
run buildall |
3fd5472 to
0de5285
Compare
|
run buildall |
|
clang-tidy review says "All clean, LGTM! 👍" |
0de5285 to
bc5ec47
Compare
|
clang-tidy review says "All clean, LGTM! 👍" |
| Status st = io::global_local_filesystem()->iterate_directory(ss.str(), cb); | ||
| if (!st.ok()) { | ||
| LOG(WARNING) << "failed to count fd: " << st; | ||
| std::error_code ec; |
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.
remove these unused shadow variable
be/src/util/doris_metrics.cpp
Outdated
| return; | ||
| } | ||
| int64_t count = std::count_if(dict_iter, std::filesystem::fs::directory_iterator(), [](const auto& entry) { | ||
| std::error_code ec; |
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.
not use shadow variable, just use dec or something else
be/src/util/doris_metrics.cpp
Outdated
| process_fd_num_used->set_value(0); | ||
| return; | ||
| } | ||
| int64_t count = std::count_if(dict_iter, std::filesystem::directory_iterator(), [](const auto& entry) { |
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.
use std::filesystem::end(dict_iter) to replace std::filesystem::directory_iterator(), std::filesystem::directory_iterator() is ub in end
bc5ec47 to
533fe3c
Compare
|
clang-tidy review says "All clean, LGTM! 👍" |
|
run buildall |
|
TeamCity be ut coverage result: |
|
(From new machine)TeamCity pipeline, clickbench performance test result: |
JackDrogon
left a comment
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
morningman
left a comment
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
|
PR approved by at least one committer and no changes requested. |
…file error indiscriminately. (apache#26013) Avoid `update_process_fd_num` reporting `no such file error `indiscriminately.
…file error indiscriminately. (apache#26013) Avoid `update_process_fd_num` reporting `no such file error `indiscriminately.
…file error indiscriminately apache#26013 (apache#26373)
…file error indiscriminately. (apache#26013) Avoid `update_process_fd_num` reporting `no such file error `indiscriminately.
Proposed changes
Avoid
update_process_fd_numreportingno such file errorindiscriminately.Further comments
If this is a relatively large or complex change, kick off the discussion at dev@doris.apache.org by explaining why you chose the solution you did and what alternatives you considered, etc...