Skip to content
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

LogCleaner: avoid scanning logs too frequently #776

Merged
merged 1 commit into from
Jan 25, 2022

Conversation

TommyWu-fdgkhdkgh
Copy link
Contributor

This patch is for the performance issue : #753.

Originally, "LogCleaner" scanned all the log directories every time we flush. In the common case, we flush every 30 seconds and then called "LogCleaner::Run". But if we flush every time we do "LogFileObject::Write" like the test code from @kimi20071025, we'll call "LogCleaner::Run" so many times.

In this patch, I maintain an additional timer next_cleanup_time_ ( thank @aesophor for the advice ), and avoid scanning the logs too frequently.

@codecov-commenter
Copy link

codecov-commenter commented Dec 30, 2021

Codecov Report

Merging #776 (4abfb55) into master (a8cfbe0) will increase coverage by 0.07%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #776      +/-   ##
==========================================
+ Coverage   72.65%   72.72%   +0.07%     
==========================================
  Files          17       17              
  Lines        3247     3256       +9     
==========================================
+ Hits         2359     2368       +9     
  Misses        888      888              
Impacted Files Coverage Δ
src/logging.cc 73.57% <100.00%> (+0.17%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a8cfbe0...4abfb55. Read the comment docs.

Copy link
Collaborator

@sergiud sergiud left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR. Please take a look at the contribution guide and make sure to follow it. Particularly, you must sign the CLA.

src/logging.cc Show resolved Hide resolved
src/logging.cc Outdated Show resolved Hide resolved
src/logging.cc Outdated Show resolved Hide resolved
@@ -503,6 +510,7 @@ class LogCleaner {

bool enabled_;
unsigned int overdue_days_;
int64 next_cleanup_time_; // cycle count at which to clean overdue log
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As before: why using a signed integer?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As before: I use integer because another timer ( e.g. next_flush_time_ ) use integer, too.

src/logging.cc Outdated Show resolved Hide resolved
@sergiud
Copy link
Collaborator

sergiud commented Jan 4, 2022

@fdgkhdkgh Some comments are still open. Please let me know if you need some clarification.

@aesophor
Copy link
Contributor

aesophor commented Jan 5, 2022

@fdgkhdkgh Thank you for adding this!

@sergiud
Copy link
Collaborator

sergiud commented Jan 24, 2022

This looks good to me now (unless @aesophor has any objections).

@fdgkhdkgh Would you like to add yourself to the AUTHORS and CONTRIBUTORS file as per contribution guide before we merge this PR?

@aesophor
Copy link
Contributor

LGTM. Thanks so much! 😄

@sergiud sergiud added this to the 0.6 milestone Jan 24, 2022
@sergiud
Copy link
Collaborator

sergiud commented Jan 25, 2022

Thanks!

@sergiud sergiud merged commit 1883610 into google:master Jan 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants