-
Notifications
You must be signed in to change notification settings - Fork 4.5k
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
Daily rotation does not remove oldest files if one day is missing #2553
Comments
Or remove the break. The problem is that in both cases it might take long time. Depending on the |
You can't remove the break: when there are less than max_files_ files, the while loop can't exit. |
I think the cleaning function could be fixed to stop relying on the PR is welcome. |
I agree. |
I was thinking that we can't guarantee that there won't be a ton of files to look through, so what if we just allowed a certain number of missing files in the search, something like: int allowed_missing = 32;
while (filenames.size() < max_files_)
{
auto filename = FileNameCalc::calc_filename(base_filename_, now_tm(now));
if (path_exists(filename))
{
filenames.emplace_back(filename);
} else if (--allowed_missing <= 0)
{
break;
}
now -= std::chrono::hours(24);
} I know it' not very robust, but is that a sensible compromise? Otherwise, I'd be willing to implement searching for files to remove. It seems like we could repurpose code from |
@andy-byers this could be a partial solution, but you should have the const To manage the general case, you would end up with an So, what's the worse between:
I've no doubt the best solution is to check all the files in the directory for log files name. |
I second that. |
@daniele77 and @gabime: that makes sense to me. I'll start a fork and see what I can do. Thanks! |
If checking the name of all the files in the directory is inevitable maybe it's worth to divide max_files_ into two different N:
This will extend sink options and simplify logic for creator (date or number but not both ). Strategy for each N can be injected into daily_file_sink constructor. |
If for any reason one of the old files is missing (e.g., because one day the application has not logged), the oldest file is no more removed.
For example, having these files:
myapp_2022-11-21.log and myapp_2022-11-20.log won't be never deleted.
This function causes the issue:
https://github.com/gabime/spdlog/blob/v1.x/include/spdlog/sinks/daily_file_sink.h#L183
I guess one fix could be to change the function so that it enumerates the files with the right pattern and remove the oldest one based on the file timestamp.
The text was updated successfully, but these errors were encountered: