feat:Added the periodically clearing serverlog function #2829
Conversation
WalkthroughThe recent updates introduce a log retention feature in the Pika server's configuration, enabling automatic management of log files based on a specified period. This enhancement prevents excessive disk usage. The log purging functionalities have been refined, with methods specifically separated for server logs and binary logs, promoting clearer and more efficient log handling throughout the system. Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range, codebase verification and nitpick comments (3)
src/pika_server.cc (3)
1216-1218: Consider logging when retention time is not set.When
retention_timeis less than or equal to 0, consider logging a message to indicate that log purging is disabled.if (retention_time <= 0) { + LOG(INFO) << "Log purging is disabled as retention time is not set."; return; }
1225-1227: Handle potential errors inGetChildren.The method should handle potential errors when retrieving the list of log files. Consider logging an error message if
GetChildrenfails.if (pstd::GetChildren(log_path, log_files) != 0) { + LOG(ERROR) << "Failed to retrieve log files from " << log_path; return; }
1247-1250: Handle potential errors in date extraction.The method should handle potential errors when extracting the date and time from the log filenames. Consider logging an error message if the extraction fails.
if (date_pos == std::string::npos || date_pos + 16 > file.length()) { + LOG(ERROR) << "Failed to extract date and time from log filename: " << file; continue; }
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (4)
- conf/pika.conf (1 hunks)
- include/pika_conf.h (2 hunks)
- include/pika_server.h (1 hunks)
- src/pika_server.cc (2 hunks)
Files skipped from review due to trivial changes (1)
- conf/pika.conf
Additional comments not posted (10)
include/pika_server.h (2)
516-516: MethodAutoBinlogPurgelooks good.The method
AutoBinlogPurgehas been correctly added and is expected to handle the purging of binary logs.
517-517: MethodAutoServerlogPurgelooks good.The method
AutoServerlogPurgehas been correctly added and is expected to handle the purging of server logs.include/pika_conf.h (2)
80-83: Methodlog_retention_timelooks good.The method
log_retention_timehas been correctly added and ensures thread-safe access to thelog_retention_time_variable.
911-911: Member variablelog_retention_time_looks good.The member variable
log_retention_time_has been correctly added to store the log retention time configuration.src/pika_server.cc (6)
1088-1091: LGTM! But verify the new method calls.The integration of
AutoServerlogPurgeandAutoBinlogPurgewithinDoTimingTasklooks good.However, ensure that these methods are correctly implemented and tested.
1211-1212: LGTM! But verifyDoSameThingEveryDBfunctionality.The
AutoBinlogPurgemethod looks good.However, ensure that
DoSameThingEveryDBcorrectly handles the purging of binlogs.
1213-1215: Ensurelog_retention_timeis correctly configured.The method relies on
log_retention_timefrom the configuration. Ensure this value is correctly set and validated.
1221-1223: Ensure log path exists.The method checks if the log path exists. Ensure that the log path is correctly configured and accessible.
1235-1238: Verify log filename format.Ensure that the log filenames follow the expected format (
pika.[hostname].[user name].log.[severity level].[date].[time].[pid]).
1280-1284: LGTM! Ensure proper testing.The logic for deleting outdated log files looks good.
Ensure that this functionality is thoroughly tested to avoid accidental deletion of important log files.
src/pika_server.cc
Outdated
| if (interval_days > retention_time) { | ||
| std::string log_file = log_path + "/" + file; | ||
| LOG(INFO) << "Deleting out of date log file: " << log_file; | ||
| pstd::DeleteFile(log_file); |
There was a problem hiding this comment.
pls check return value and print Error Log if deleteFile failed
src/pika_server.cc
Outdated
| int64_t now_timestamp = mktime(&now_time); | ||
| int64_t interval_days = (now_timestamp - log_timestamp) / 86400; | ||
|
|
||
| if (interval_days > retention_time) { |
There was a problem hiding this comment.
A logic is needed to guarantee that the latest log(which is in use) will not be deleted even if it's logically expired.
The time extracted from log file name is the time of it's creation. thus it's possible that a log has logically expired but still in use if the pika server is inactive.
There was a problem hiding this comment.
Do your mean that assuming a Pika instance has been running continuously for 8 days, a server log created 8 days ago should logically have expired, but in reality, it is the most recent and currently used server log?
Check return value and print Error Log if deleteFile failed
| // Purge serverlog | ||
| AutoServerlogPurge(); | ||
| // Purge binlog | ||
| AutoBinlogPurge(); |
There was a problem hiding this comment.
Ensure proper error handling for purging tasks.
Consider adding error handling and logging for AutoServerlogPurge and AutoBinlogPurge to capture any issues during log purging.
// Purge serverlog
if (!AutoServerlogPurge()) {
LOG(ERROR) << "Failed to purge server logs";
}
// Purge binlog
if (!AutoBinlogPurge()) {
LOG(ERROR) << "Failed to purge binlogs";
}
src/pika_server.cc
Outdated
| void PikaServer::AutoServerlogPurge(){ | ||
| std::string log_path = g_pika_conf->log_path(); | ||
| int retention_time = g_pika_conf->log_retention_time(); | ||
| if (retention_time <= 0) { | ||
| return; | ||
| } | ||
| std::vector<std::string> log_files; | ||
|
|
||
| if (!pstd::FileExists(log_path)) { | ||
| return; | ||
| } | ||
|
|
||
| if (pstd::GetChildren(log_path, log_files) != 0) { | ||
| return; | ||
| } | ||
|
|
||
| time_t t = time(nullptr); | ||
| struct tm* now = localtime(&t); | ||
| int now_year = now->tm_year + 1900; | ||
| int now_month = now->tm_mon + 1; | ||
| int now_day = now->tm_mday; | ||
|
|
||
| //logformat: pika.[hostname].[user name].log.[severity level].[date].[time].[pid] | ||
| for (const auto& file : log_files) { | ||
| if (file.substr(0, 5) != "pika.") { | ||
| continue; | ||
| } | ||
|
|
||
| size_t log_pos = file.find(".log."); | ||
| if (log_pos == std::string::npos) { | ||
| continue; | ||
| } | ||
|
|
||
| // Start at the end of ".log." to find the date and time | ||
| size_t date_pos = file.find('.', log_pos + 5); | ||
| if (date_pos == std::string::npos || date_pos + 16 > file.length()) { | ||
| continue; | ||
| } | ||
|
|
||
| std::string date = file.substr(date_pos + 1, 8); | ||
| std::string time = file.substr(date_pos + 10, 6); | ||
|
|
||
| int log_year = std::atoi(date.substr(0, 4).c_str()); | ||
| int log_month = std::atoi(date.substr(4, 2).c_str()); | ||
| int log_day = std::atoi(date.substr(6, 2).c_str()); | ||
|
|
||
| struct tm log_time; | ||
| struct tm now_time; | ||
|
|
||
| log_time.tm_year = log_year - 1900; | ||
| log_time.tm_mon = log_month - 1; | ||
| log_time.tm_mday = log_day; | ||
| log_time.tm_hour = 0; | ||
| log_time.tm_min = 0; | ||
| log_time.tm_sec = 0; | ||
|
|
||
| now_time.tm_year = now_year - 1900; | ||
| now_time.tm_mon = now_month - 1; | ||
| now_time.tm_mday = now_day; | ||
| now_time.tm_hour = 0; | ||
| now_time.tm_min = 0; | ||
| now_time.tm_sec = 0; | ||
|
|
||
| int64_t log_timestamp = mktime(&log_time); | ||
| int64_t now_timestamp = mktime(&now_time); | ||
| int64_t interval_days = (now_timestamp - log_timestamp) / 86400; | ||
|
|
||
| if (interval_days > retention_time) { | ||
| std::string log_file = log_path + "/" + file; | ||
| LOG(INFO) << "Deleting out of date log file: " << log_file; | ||
| if (!pstd::DeleteFile(log_file)) { | ||
| LOG(ERROR) << "Failed to delete log file: " << log_file; | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Add error handling and logging for file operations.
Ensure that all file operations have proper error handling and logging.
void PikaServer::AutoServerlogPurge() {
std::string log_path = g_pika_conf->log_path();
int retention_time = g_pika_conf->log_retention_time();
if (retention_time <= 0) {
return;
}
std::vector<std::string> log_files;
if (!pstd::FileExists(log_path)) {
LOG(WARNING) << "Log path does not exist: " << log_path;
return;
}
if (pstd::GetChildren(log_path, log_files) != 0) {
LOG(ERROR) << "Failed to retrieve log files from: " << log_path;
return;
}
time_t t = time(nullptr);
struct tm* now = localtime(&t);
int now_year = now->tm_year + 1900;
int now_month = now->tm_mon + 1;
int now_day = now->tm_mday;
// log format: pika.[hostname].[user name].log.[severity level].[date].[time].[pid]
for (const auto& file : log_files) {
if (file.substr(0, 5) != "pika.") {
continue;
}
size_t log_pos = file.find(".log.");
if (log_pos == std::string::npos) {
continue;
}
// Start at the end of ".log." to find the date and time
size_t date_pos = file.find('.', log_pos + 5);
if (date_pos == std::string::npos || date_pos + 16 > file.length()) {
continue;
}
std::string date = file.substr(date_pos + 1, 8);
std::string time = file.substr(date_pos + 10, 6);
int log_year = std::atoi(date.substr(0, 4).c_str());
int log_month = std::atoi(date.substr(4, 2).c_str());
int log_day = std::atoi(date.substr(6, 2).c_str());
struct tm log_time;
struct tm now_time;
log_time.tm_year = log_year - 1900;
log_time.tm_mon = log_month - 1;
log_time.tm_mday = log_day;
log_time.tm_hour = 0;
log_time.tm_min = 0;
log_time.tm_sec = 0;
now_time.tm_year = now_year - 1900;
now_time.tm_mon = now_month - 1;
now_time.tm_mday = now_day;
now_time.tm_hour = 0;
now_time.tm_min = 0;
now_time.tm_sec = 0;
int64_t log_timestamp = mktime(&log_time);
int64_t now_timestamp = mktime(&now_time);
int64_t interval_days = (now_timestamp - log_timestamp) / 86400;
if (interval_days > retention_time) {
std::string log_file = log_path + "/" + file;
LOG(INFO) << "Deleting out of date log file: " << log_file;
if (!pstd::DeleteFile(log_file)) {
LOG(ERROR) << "Failed to delete log file: " << log_file;
}
}
}
}
src/pika_server.cc
Outdated
| int64_t now_timestamp = mktime(&now_time); | ||
| int64_t interval_days = (now_timestamp - log_timestamp) / 86400; | ||
|
|
||
| if (interval_days > retention_time) { |
There was a problem hiding this comment.
Ensure the latest log file is not deleted if still in use.
Add logic to guarantee that the latest log file, which might still be in use, is not deleted even if it is logically expired.
if (interval_days > retention_time && !IsLogFileInUse(file)) {
std::string log_file = log_path + "/" + file;
LOG(INFO) << "Deleting out of date log file: " << log_file;
if (!pstd::DeleteFile(log_file)) {
LOG(ERROR) << "Failed to delete log file: " << log_file;
}
}
1. retain the newest serverlog file even if it has been expired 2. Make sure that serverlogs of [INFO],[ERROR], and [WARNING] levels are retained
|
目前看着代码是OK的，能否辛苦实测一下呢，可以先手动mock一下代码，把过期时间改成10分钟，临时添加一些日志输出（比如每条指令都打一次日志，然后用benchmark压，很快就会累计大量日志，并且发生切换），主要测试一下功能正确性。 因为要上实际生产，所以需要稳妥一些，辛苦了！ |
At present, the code seems to be OK. Can you test it manually? You can manually mock the code first, change the expiration time to 10 minutes, and temporarily add some log output (for example, log each instruction once, and then use benchmark to press it. A large amount of logs will accumulate soon, and switching will occur), mainly to test the correctness of the function. Because we have to go into actual production, we need to be more cautious. Thank you for your hard work! |
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- conf/pika.conf (1 hunks)
- include/pika_conf.h (2 hunks)
- src/pika_server.cc (2 hunks)
Files skipped from review due to trivial changes (1)
- conf/pika.conf
Files skipped from review as they are similar to previous changes (1)
- include/pika_conf.h
Additional comments not posted (4)
src/pika_server.cc (4)
1090-1093: Ensure proper error handling for purging tasks.Consider adding error handling and logging for
AutoServerlogPurgeandAutoBinlogPurgeto capture any issues during log purging.// Purge serverlog if (!AutoServerlogPurge()) { LOG(ERROR) << "Failed to purge server logs"; } // Purge binlog if (!AutoBinlogPurge()) { LOG(ERROR) << "Failed to purge binlogs"; }
1213-1214: Add error handling and logging.Consider adding error handling and logging to capture any issues during binlog purging.
void PikaServer::AutoBinlogPurge() { Status s = DoSameThingEveryDB(TaskType::kPurgeLog); if (!s.ok()) { LOG(ERROR) << "Failed to purge binlogs: " << s.ToString(); } }
1215-1297: Add error handling and logging for file operations.Ensure that all file operations have proper error handling and logging.
void PikaServer::AutoServerlogPurge() { std::string log_path = g_pika_conf->log_path(); int retention_time = g_pika_conf->log_retention_time(); if (retention_time <= 0) { return; } std::vector<std::string> log_files; if (!pstd::FileExists(log_path)) { LOG(WARNING) << "Log path does not exist: " << log_path; return; } if (pstd::GetChildren(log_path, log_files) != 0) { LOG(ERROR) << "Failed to retrieve log files from: " << log_path; return; } time_t t = time(nullptr); struct tm* now = localtime(&t); int now_year = now->tm_year + 1900; int now_month = now->tm_mon + 1; int now_day = now->tm_mday; // log format: pika.[hostname].[user name].log.[severity level].[date].[time].[pid] for (const auto& file : log_files) { if (file.substr(0, 5) != "pika.") { continue; } size_t log_pos = file.find(".log."); if (log_pos == std::string::npos) { continue; } // Start at the end of ".log." to find the date and time size_t date_pos = file.find('.', log_pos + 5); if (date_pos == std::string::npos || date_pos + 16 > file.length()) { continue; } std::string date = file.substr(date_pos + 1, 8); std::string time = file.substr(date_pos + 10, 6); int log_year = std::atoi(date.substr(0, 4).c_str()); int log_month = std::atoi(date.substr(4, 2).c_str()); int log_day = std::atoi(date.substr(6, 2).c_str()); struct tm log_time; struct tm now_time; log_time.tm_year = log_year - 1900; log_time.tm_mon = log_month - 1; log_time.tm_mday = log_day; log_time.tm_hour = 0; log_time.tm_min = 0; log_time.tm_sec = 0; now_time.tm_year = now_year - 1900; now_time.tm_mon = now_month - 1; now_time.tm_mday = now_day; now_time.tm_hour = 0; now_time.tm_min = 0; now_time.tm_sec = 0; int64_t log_timestamp = mktime(&log_time); int64_t now_timestamp = mktime(&now_time); int64_t interval_days = (now_timestamp - log_timestamp) / 86400; if (interval_days > retention_time) { std::string log_file = log_path + "/" + file; LOG(INFO) << "Deleting out of date log file: " << log_file; if (!pstd::DeleteFile(log_file)) { LOG(ERROR) << "Failed to delete log file: " << log_file; } } } }
1215-1297: Ensure the latest log file is not deleted if still in use.Add logic to guarantee that the latest log file, which might still be in use, is not deleted even if it is logically expired.
if (interval_days > retention_time && !IsLogFileInUse(file)) { std::string log_file = log_path + "/" + file; LOG(INFO) << "Deleting out of date log file: " << log_file; if (!pstd::DeleteFile(log_file)) { LOG(ERROR) << "Failed to delete log file: " << log_file; } }
ok,I got it.I have done a simple test before, and I will do a more detailed test according to the procedure you described |
Adjust the time calculation method
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- src/pika_server.cc (2 hunks)
Additional comments not posted (4)
src/pika_server.cc (4)
1090-1093: Ensure proper error handling for purging tasks.Consider adding error handling and logging for
AutoServerlogPurgeandAutoBinlogPurgeto capture any issues during log purging.// Purge serverlog if (!AutoServerlogPurge()) { LOG(ERROR) << "Failed to purge server logs"; } // Purge binlog if (!AutoBinlogPurge()) { LOG(ERROR) << "Failed to purge binlogs"; }
1213-1214: Add error handling and logging.Consider adding error handling and logging to capture any issues during binlog purging.
void PikaServer::AutoBinlogPurge() { Status s = DoSameThingEveryDB(TaskType::kPurgeLog); if (!s.ok()) { LOG(ERROR) << "Failed to purge binlogs: " << s.ToString(); } }
1215-1289: Add error handling and logging for file operations.Ensure that all file operations have proper error handling and logging.
void PikaServer::AutoServerlogPurge() { std::string log_path = g_pika_conf->log_path(); int retention_time = g_pika_conf->log_retention_time(); if (retention_time <= 0) { return; } std::vector<std::string> log_files; if (!pstd::FileExists(log_path)) { LOG(WARNING) << "Log path does not exist: " << log_path; return; } if (pstd::GetChildren(log_path, log_files) != 0) { LOG(ERROR) << "Failed to retrieve log files from: " << log_path; return; } time_t t = time(nullptr); struct tm* now = localtime(&t); int now_year = now->tm_year + 1900; int now_month = now->tm_mon + 1; int now_day = now->tm_mday; // log format: pika.[hostname].[user name].log.[severity level].[date].[time].[pid] for (const auto& file : log_files) { if (file.substr(0, 5) != "pika.") { continue; } size_t log_pos = file.find(".log."); if (log_pos == std::string::npos) { continue; } // Start at the end of ".log." to find the date and time size_t date_pos = file.find('.', log_pos + 5); if (date_pos == std::string::npos || date_pos + 16 > file.length()) { continue; } std::string date = file.substr(date_pos + 1, 8); std::string time = file.substr(date_pos + 10, 6); int log_year = std::atoi(date.substr(0, 4).c_str()); int log_month = std::atoi(date.substr(4, 2).c_str()); int log_day = std::atoi(date.substr(6, 2).c_str()); struct tm log_time; struct tm now_time; log_time.tm_year = log_year - 1900; log_time.tm_mon = log_month - 1; log_time.tm_mday = log_day; log_time.tm_hour = 0; log_time.tm_min = 0; log_time.tm_sec = 0; now_time.tm_year = now_year - 1900; now_time.tm_mon = now_month - 1; now_time.tm_mday = now_day; now_time.tm_hour = 0; now_time.tm_min = 0; now_time.tm_sec = 0; int64_t log_timestamp = mktime(&log_time); int64_t now_timestamp = mktime(&now_time); int64_t interval_days = (now_timestamp - log_timestamp) / 86400; if (interval_days > retention_time) { std::string log_file = log_path + "/" + file; LOG(INFO) << "Deleting out of date log file: " << log_file; if (!pstd::DeleteFile(log_file)) { LOG(ERROR) << "Failed to delete log file: " << log_file; } } } }
1215-1289: Ensure the latest log file is not deleted if still in use.Add logic to guarantee that the latest log file, which might still be in use, is not deleted even if it is logically expired.
if (interval_days > retention_time && !IsLogFileInUse(file)) { std::string log_file = log_path + "/" + file; LOG(INFO) << "Deleting out of date log file: " << log_file; if (!pstd::DeleteFile(log_file)) { LOG(ERROR) << "Failed to delete log file: " << log_file; } }
|
I ran more detailed tests,It seems to be working rightly. Test approach:
|
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range, codebase verification and nitpick comments (1)
src/pika_server.cc (1)
1215-1218: Add error handling for negative retention time.Currently, the function returns if
retention_timeis less than 0. Consider logging a warning message to indicate that the retention time is invalid.if (retention_time < 0) { LOG(WARNING) << "Invalid log retention time: " << retention_time; return; }
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- src/pika_server.cc (2 hunks)
Additional comments not posted (3)
src/pika_server.cc (3)
1090-1093: Ensure proper error handling for purging tasks.Consider adding error handling and logging for
AutoServerlogPurgeandAutoBinlogPurgeto capture any issues during log purging.// Purge serverlog if (!AutoServerlogPurge()) { LOG(ERROR) << "Failed to purge server logs"; } // Purge binlog if (!AutoBinlogPurge()) { LOG(ERROR) << "Failed to purge binlogs"; }
1223-1229: Add error handling and logging for file operations.Ensure that all file operations have proper error handling and logging to capture any issues during log file retrieval.
if (!pstd::FileExists(log_path)) { LOG(WARNING) << "Log path does not exist: " << log_path; return; } if (pstd::GetChildren(log_path, log_files) != 0) { LOG(ERROR) << "Failed to retrieve log files from: " << log_path; return; }
1285-1288: Ensure the latest log file is not deleted if still in use.Add logic to guarantee that the latest log file, which might still be in use, is not deleted even if it is logically expired.
if (interval_days > retention_time && !IsLogFileInUse(file)) { std::string log_file = log_path + "/" + file; LOG(INFO) << "Deleting out of date log file: " << log_file; if (!pstd::DeleteFile(log_file)) { LOG(ERROR) << "Failed to delete log file: " << log_file; } }
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range, codebase verification and nitpick comments (1)
src/pika_conf.cc (1)
131-133: Enhance error message for invalidlog-retention-time.The error message could be more descriptive to aid debugging and user understanding. Consider including the invalid value in the log message.
- LOG(FATAL) << "log-retention-time invalid"; + LOG(FATAL) << "log-retention-time invalid: " << log_retention_time_;
* Added the serverlog periodic reclamation function --------- Co-authored-by: cheniujh <[email protected]>
…dation#2829) * Added the serverlog periodic reclamation function --------- Co-authored-by: cheniujh <[email protected]>
…dation#2829) * Added the serverlog periodic reclamation function --------- Co-authored-by: cheniujh <[email protected]>
#2811
Configure the log-retention-time field in pika.conf. Serverlogs that exceed this time period are cleared
Summary by CodeRabbit
New Features
Bug Fixes