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
Implement support of date/time format specifiers in log and error log file names #51945
Conversation
This is an automated comment for commit e592c7c with description of existing statuses. It's updated for the latest CI running
|
Hi, could anyone be assigned to this PR? So Victor could start working on reviewers suggestions. |
LGTM. The |
localtime_r(&now, &buf); | ||
std::ostringstream ss; // STYLE_CHECK_ALLOW_STD_STRING_STREAM | ||
ss << std::put_time(&buf, file_path.c_str()); | ||
return path.replace_filename(ss.str()); |
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 breaks relative path configuration for non-daemon servers. Consider the following example:
<logger>
<level>trace</level>
<log>logs/clickhouse-server.log</log>
<errorlog>logs/clickhouse-server.err.log</errorlog>
<size>1000M</size>
<count>10</count>
</logger>
The log path will be modified as logs/logs/clickhouse-server.log
.
cc @sirvickr
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.
That's the exact reason why those format specifiers currently do not apply to the "path" part - only to the file name.
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.
@amosbird, path.replace_filename()
does the trick.
If someone decides another day to apply the specifiers to the full path, they will have to take into consideration this issue about non-daemon servers.
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.
That's the exact reason why those format specifiers currently do not apply to the "path" part - only to the file name.
@sirvickr I'm confused. If that's the expected behavior, why not do the following:
ss << std::put_time(&buf, path.filename().c_str());
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.
@amosbird, you are right, my code looks like an ugly typo, to say the least. This one is more concise, clear and correct:
ss << std::put_time(&buf, path.filename().c_str());
It looks like it should be amended.
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 guess, replace_filename()
extracts the file name from its argument, otherwise it wouldn't work properly.
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 thank you for noticing.
I'm thinking of creating a "follow-up" PR to clean up this line of code. Or may be you'd rather amend this code by yourself?
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 thank you for noticing.
No problem!
I'm thinking of creating a "follow-up" PR to clean up this line of code.
Perfect. Please go ahead for amending it :)
Changelog category (leave one):
Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):
Added the possibility to use date and time format specifiers in log and error log file names, either in config files (
log
anderrorlog
tags) or command line arguments (--log-file
and--errorlog-file
)Documentation entry for user-facing changes