You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@kvrocks.apache.org by GitBox <gi...@apache.org> on 2022/12/08 16:09:36 UTC
[GitHub] [incubator-kvrocks] IoCing opened a new pull request, #1171: add config about logcleaner
IoCing opened a new pull request, #1171:
URL: https://github.com/apache/incubator-kvrocks/pull/1171
refer to #1168
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] IoCing commented on a diff in pull request #1171: add config about logcleaner
Posted by GitBox <gi...@apache.org>.
IoCing commented on code in PR #1171:
URL: https://github.com/apache/incubator-kvrocks/pull/1171#discussion_r1044032616
##########
src/main.cc:
##########
@@ -178,6 +178,15 @@ static void initGoogleLog(const Config *config) {
FLAGS_logtostdout = true;
} else {
FLAGS_log_dir = config->log_dir;
+ if (config->enablelogcleaner) {
+ google::EnableLogCleaner(config->logcleanerday);
+ google::SetLogFilenameExtension(".log");
+ for (int level = google::INFO; level <= google::FATAL; level++) {
+ std::string des = google::LogSeverityNames[level];
+ des = config->log_dir + "/kvrocks_" + des + "_";
+ google::SetLogDestination(level, des.c_str());
+ }
Review Comment:
OK ， i will try to resolve it。
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] tisonkun commented on pull request #1171: feat: add config for log cleaner
Posted by GitBox <gi...@apache.org>.
tisonkun commented on PR #1171:
URL: https://github.com/apache/incubator-kvrocks/pull/1171#issuecomment-1361158197
@IoCing Thank you!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] git-hulk commented on a diff in pull request #1171: add config about logcleaner
Posted by GitBox <gi...@apache.org>.
git-hulk commented on code in PR #1171:
URL: https://github.com/apache/incubator-kvrocks/pull/1171#discussion_r1044056727
##########
kvrocks.conf:
##########
@@ -96,6 +96,15 @@ dir /tmp/kvrocks
# We also can send logs to stdout/stderr is as simple as:
#
log-dir stdout
+# log-dir log
+
+# You can configure logcleanerday to control whether to enable logcleaner
+# and the maximum number of days that the INFO level logs will be kept.
+# if set logcleanerday to -1 , this mean disable logcleaner.
+# if set logcleanerday between 0 to INT_MAX , this mean enable logcleaner ,
+# and INFO level log file whose last modified time is greater than logcleanerday will be unlinked.
+# By default the logcleanerday is -1.
+logcleanerday -1
Review Comment:
So it means won't retent any logs except the current one？
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] caipengbo commented on a diff in pull request #1171: add config about logcleaner
Posted by GitBox <gi...@apache.org>.
caipengbo commented on code in PR #1171:
URL: https://github.com/apache/incubator-kvrocks/pull/1171#discussion_r1044082738
##########
src/config/config.cc:
##########
@@ -159,6 +159,7 @@ Config::Config() {
{"migrate-sequence-gap", false, new IntField(&sequence_gap, 10000, 1, INT_MAX)},
{"unixsocket", true, new StringField(&unixsocket, "")},
{"unixsocketperm", true, new OctalField(&unixsocketperm, 0777, 1, INT_MAX)},
+ {"logcleanerday", false, new IntField(&logcleanerday, -1, -1, INT_MAX)},
Review Comment:
> I prefer to use retention instead of overdue since it looks more clear than overdue. To see if other folks have thoughts on this.
I also think `retention` is better, which is a more general word.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] caipengbo commented on a diff in pull request #1171: add config about logcleaner
Posted by GitBox <gi...@apache.org>.
caipengbo commented on code in PR #1171:
URL: https://github.com/apache/incubator-kvrocks/pull/1171#discussion_r1044051461
##########
kvrocks.conf:
##########
@@ -96,6 +96,15 @@ dir /tmp/kvrocks
# We also can send logs to stdout/stderr is as simple as:
#
log-dir stdout
+# log-dir log
+
+# You can configure logcleanerday to control whether to enable logcleaner
+# and the maximum number of days that the INFO level logs will be kept.
+# if set logcleanerday to -1 , this mean disable logcleaner.
+# if set logcleanerday between 0 to INT_MAX , this mean enable logcleaner ,
+# and INFO level log file whose last modified time is greater than logcleanerday will be unlinked.
+# By default the logcleanerday is -1.
+logcleanerday -1
Review Comment:
What does 0 mean？
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] IoCing commented on a diff in pull request #1171: add config about logcleaner
Posted by GitBox <gi...@apache.org>.
IoCing commented on code in PR #1171:
URL: https://github.com/apache/incubator-kvrocks/pull/1171#discussion_r1044062653
##########
src/config/config.cc:
##########
@@ -159,6 +159,7 @@ Config::Config() {
{"migrate-sequence-gap", false, new IntField(&sequence_gap, 10000, 1, INT_MAX)},
{"unixsocket", true, new StringField(&unixsocket, "")},
{"unixsocketperm", true, new OctalField(&unixsocketperm, 0777, 1, INT_MAX)},
+ {"logcleanerday", false, new IntField(&logcleanerday, -1, -1, INT_MAX)},
Review Comment:
how about renaming it to log-overdue-days ，becaue i find related var in the glog is named overdue_days_
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] IoCing commented on a diff in pull request #1171: add config about logcleaner
Posted by GitBox <gi...@apache.org>.
IoCing commented on code in PR #1171:
URL: https://github.com/apache/incubator-kvrocks/pull/1171#discussion_r1044060860
##########
src/config/config.cc:
##########
@@ -159,6 +159,7 @@ Config::Config() {
{"migrate-sequence-gap", false, new IntField(&sequence_gap, 10000, 1, INT_MAX)},
{"unixsocket", true, new StringField(&unixsocket, "")},
{"unixsocketperm", true, new OctalField(&unixsocketperm, 0777, 1, INT_MAX)},
+ {"logcleanerday", false, new IntField(&logcleanerday, -1, -1, INT_MAX)},
Review Comment:
ok ， i will do it
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] git-hulk commented on a diff in pull request #1171: feat: add config for log cleaner
Posted by GitBox <gi...@apache.org>.
git-hulk commented on code in PR #1171:
URL: https://github.com/apache/incubator-kvrocks/pull/1171#discussion_r1051832429
##########
kvrocks.conf:
##########
@@ -97,6 +97,16 @@ dir /tmp/kvrocks
#
log-dir stdout
+# You can configure log-retention-days to control whether to enable the log cleaner
+# and the maximum retention days that the INFO level logs will be kept.
+#
+# if set to -1, that means to disable the log cleaner.
+# if set to 0, all previous INFO level logs will be immediately removed.
Review Comment:
@IoCing I think we can move on with all logs with the same retention behavior first, then optimize it if necessary.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] git-hulk commented on a diff in pull request #1171: add config about logcleaner
Posted by GitBox <gi...@apache.org>.
git-hulk commented on code in PR #1171:
URL: https://github.com/apache/incubator-kvrocks/pull/1171#discussion_r1044063552
##########
src/config/config.cc:
##########
@@ -159,6 +159,7 @@ Config::Config() {
{"migrate-sequence-gap", false, new IntField(&sequence_gap, 10000, 1, INT_MAX)},
{"unixsocket", true, new StringField(&unixsocket, "")},
{"unixsocketperm", true, new OctalField(&unixsocketperm, 0777, 1, INT_MAX)},
+ {"logcleanerday", false, new IntField(&logcleanerday, -1, -1, INT_MAX)},
Review Comment:
I prefer to use `retention` instead of overdue since it looks more clear than overdue. To see if other folks have thoughts on this. @PragmaTwice @torwig @caipengbo
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] IoCing commented on a diff in pull request #1171: feat: add config for log cleaner
Posted by GitBox <gi...@apache.org>.
IoCing commented on code in PR #1171:
URL: https://github.com/apache/incubator-kvrocks/pull/1171#discussion_r1050433550
##########
kvrocks.conf:
##########
@@ -96,6 +96,15 @@ dir /tmp/kvrocks
# We also can send logs to stdout/stderr is as simple as:
#
log-dir stdout
+# log-dir log
+
+# You can configure logcleanerday to control whether to enable logcleaner
+# and the maximum number of days that the INFO level logs will be kept.
+# if set logcleanerday to -1 , this mean disable logcleaner.
+# if set logcleanerday between 0 to INT_MAX , this mean enable logcleaner ,
+# and INFO level log file whose last modified time is greater than logcleanerday will be unlinked.
+# By default the logcleanerday is -1.
+logcleanerday -1
Review Comment:
> yes ，but only INFO level will be unlinked。WARNING，ERROR，FATAL level log will be kept
Sorry , this describe is wrong.
I went to see the code of blog . I find that it will unlink all level log.
In the glog , every time a log is written, it will be judged whether to do logclean.
glog set a variable FLAGS_logcleansecs to control the time interval of logclean operation (Default is 5 minutes)。
Each level has a logfileobject , but only one logclean object. So if INFO level logfileobject
do logclean at time 0 , other level logfileobject can only logclean after five minutes.
the code of glog
https://github.com/google/glog/blob/master/src/logging.cc#:~:text=void%20LogCleaner%3A%3AUpdateCleanUpTime,UpdateCleanUpTime()%3B
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] PragmaTwice commented on a diff in pull request #1171: add config about logcleaner
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on code in PR #1171:
URL: https://github.com/apache/incubator-kvrocks/pull/1171#discussion_r1043577528
##########
src/config/config.cc:
##########
@@ -159,6 +159,8 @@ Config::Config() {
{"migrate-sequence-gap", false, new IntField(&sequence_gap, 10000, 1, INT_MAX)},
{"unixsocket", true, new StringField(&unixsocket, "")},
{"unixsocketperm", true, new OctalField(&unixsocketperm, 0777, 1, INT_MAX)},
+ {"enablelogcleaner", false, new YesNoField(&enablelogcleaner, false)},
+ {"logcleanerday", false, new IntField(&logcleanerday, 365, 0, INT_MAX)},
Review Comment:
Why the default value here is 365?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] caipengbo commented on a diff in pull request #1171: add config about logcleaner
Posted by GitBox <gi...@apache.org>.
caipengbo commented on code in PR #1171:
URL: https://github.com/apache/incubator-kvrocks/pull/1171#discussion_r1043949368
##########
kvrocks.conf:
##########
@@ -96,6 +96,15 @@ dir /tmp/kvrocks
# We also can send logs to stdout/stderr is as simple as:
#
log-dir stdout
+# log-dir log
+
+# To enable the log cleaner. if set yes , glog will check if there are
+# overdue logs whenever a flush is performed
+enablelogcleaner yes
Review Comment:
I think a single `logcleanerday` item is sufficient, with a value of 0(or -1) to turn off logcleaner.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] git-hulk commented on a diff in pull request #1171: add config about logcleaner
Posted by GitBox <gi...@apache.org>.
git-hulk commented on code in PR #1171:
URL: https://github.com/apache/incubator-kvrocks/pull/1171#discussion_r1044061572
##########
kvrocks.conf:
##########
@@ -96,6 +96,15 @@ dir /tmp/kvrocks
# We also can send logs to stdout/stderr is as simple as:
#
log-dir stdout
+# log-dir log
+
+# You can configure logcleanerday to control whether to enable logcleaner
+# and the maximum number of days that the INFO level logs will be kept.
+# if set logcleanerday to -1 , this mean disable logcleaner.
+# if set logcleanerday between 0 to INT_MAX , this mean enable logcleaner ,
+# and INFO level log file whose last modified time is greater than logcleanerday will be unlinked.
+# By default the logcleanerday is -1.
+logcleanerday -1
Review Comment:
Got it, thanks for your explanation.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] IoCing commented on a diff in pull request #1171: add config about logcleaner
Posted by GitBox <gi...@apache.org>.
IoCing commented on code in PR #1171:
URL: https://github.com/apache/incubator-kvrocks/pull/1171#discussion_r1044033191
##########
kvrocks.conf:
##########
@@ -96,6 +96,15 @@ dir /tmp/kvrocks
# We also can send logs to stdout/stderr is as simple as:
#
log-dir stdout
+# log-dir log
+
+# To enable the log cleaner. if set yes , glog will check if there are
+# overdue logs whenever a flush is performed
+enablelogcleaner yes
Review Comment:
thanks, i get it
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] IoCing commented on a diff in pull request #1171: feat: add config for log cleaner
Posted by GitBox <gi...@apache.org>.
IoCing commented on code in PR #1171:
URL: https://github.com/apache/incubator-kvrocks/pull/1171#discussion_r1044941929
##########
kvrocks.conf:
##########
@@ -97,6 +97,16 @@ dir /tmp/kvrocks
#
log-dir stdout
+# You can configure log-retention-days to control whether to enable the log cleaner
+# and the maximum retention days that the INFO level logs will be kept.
+#
+# if set to -1, that means to disable the log cleaner.
+# if set to 0, all previous INFO level logs will be immediately removed.
Review Comment:
> @IoCing Would you mind adding Go test cases for this configuration, to make sure it can be changed in-flight?
ok , i will try
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] PragmaTwice commented on a diff in pull request #1171: add config about logcleaner
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on code in PR #1171:
URL: https://github.com/apache/incubator-kvrocks/pull/1171#discussion_r1043588703
##########
src/main.cc:
##########
@@ -178,6 +178,15 @@ static void initGoogleLog(const Config *config) {
FLAGS_logtostdout = true;
} else {
FLAGS_log_dir = config->log_dir;
+ if (config->enablelogcleaner) {
+ google::EnableLogCleaner(config->logcleanerday);
+ google::SetLogFilenameExtension(".log");
+ for (int level = google::INFO; level <= google::FATAL; level++) {
+ std::string des = google::LogSeverityNames[level];
+ des = config->log_dir + "/kvrocks_" + des + "_";
+ google::SetLogDestination(level, des.c_str());
+ }
Review Comment:
I think we cannot have different log filenames just by enabling or disabling log cleaner.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] IoCing commented on pull request #1171: add config about logcleaner
Posted by GitBox <gi...@apache.org>.
IoCing commented on PR #1171:
URL: https://github.com/apache/incubator-kvrocks/pull/1171#issuecomment-1343905985
> General looks good to me, except for two suggestions.
thanks
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] IoCing commented on a diff in pull request #1171: feat: add config for log cleaner
Posted by GitBox <gi...@apache.org>.
IoCing commented on code in PR #1171:
URL: https://github.com/apache/incubator-kvrocks/pull/1171#discussion_r1050437693
##########
kvrocks.conf:
##########
@@ -97,6 +97,16 @@ dir /tmp/kvrocks
#
log-dir stdout
+# You can configure log-retention-days to control whether to enable the log cleaner
+# and the maximum retention days that the INFO level logs will be kept.
+#
+# if set to -1, that means to disable the log cleaner.
+# if set to 0, all previous INFO level logs will be immediately removed.
Review Comment:
> I read we use the same config for all logs. How can logs in other levels have different manners?
At present, glog does not provide an interface to configure the expiration time of different levels
I think it is possible to submit a pr to glog, and then we can set up accordingly。
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] git-hulk commented on a diff in pull request #1171: add config about logcleaner
Posted by GitBox <gi...@apache.org>.
git-hulk commented on code in PR #1171:
URL: https://github.com/apache/incubator-kvrocks/pull/1171#discussion_r1044056318
##########
src/config/config.cc:
##########
@@ -159,6 +159,7 @@ Config::Config() {
{"migrate-sequence-gap", false, new IntField(&sequence_gap, 10000, 1, INT_MAX)},
{"unixsocket", true, new StringField(&unixsocket, "")},
{"unixsocketperm", true, new OctalField(&unixsocketperm, 0777, 1, INT_MAX)},
+ {"logcleanerday", false, new IntField(&logcleanerday, -1, -1, INT_MAX)},
Review Comment:
We need to implement the callback function for this configuration if we expected to call to action when the value was changed. https://github.com/apache/incubator-kvrocks/blob/2ddb300b316486af20cb202c00d9c6e8fed27408/src/config/config.cc#L312
##########
kvrocks.conf:
##########
@@ -96,6 +96,15 @@ dir /tmp/kvrocks
# We also can send logs to stdout/stderr is as simple as:
#
log-dir stdout
+# log-dir log
+
+# You can configure logcleanerday to control whether to enable logcleaner
+# and the maximum number of days that the INFO level logs will be kept.
+# if set logcleanerday to -1 , this mean disable logcleaner.
+# if set logcleanerday between 0 to INT_MAX , this mean enable logcleaner ,
+# and INFO level log file whose last modified time is greater than logcleanerday will be unlinked.
+# By default the logcleanerday is -1.
+logcleanerday -1
Review Comment:
how about renaming it to `log-retention-days`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] IoCing commented on a diff in pull request #1171: add config about logcleaner
Posted by GitBox <gi...@apache.org>.
IoCing commented on code in PR #1171:
URL: https://github.com/apache/incubator-kvrocks/pull/1171#discussion_r1044055249
##########
kvrocks.conf:
##########
@@ -96,6 +96,15 @@ dir /tmp/kvrocks
# We also can send logs to stdout/stderr is as simple as:
#
log-dir stdout
+# log-dir log
+
+# You can configure logcleanerday to control whether to enable logcleaner
+# and the maximum number of days that the INFO level logs will be kept.
+# if set logcleanerday to -1 , this mean disable logcleaner.
+# if set logcleanerday between 0 to INT_MAX , this mean enable logcleaner ,
+# and INFO level log file whose last modified time is greater than logcleanerday will be unlinked.
+# By default the logcleanerday is -1.
+logcleanerday -1
Review Comment:
if set to 0 , during the flush, the previous INFO level logs will be immediately unlinked
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] IoCing commented on a diff in pull request #1171: add config about logcleaner
Posted by GitBox <gi...@apache.org>.
IoCing commented on code in PR #1171:
URL: https://github.com/apache/incubator-kvrocks/pull/1171#discussion_r1044059187
##########
kvrocks.conf:
##########
@@ -96,6 +96,15 @@ dir /tmp/kvrocks
# We also can send logs to stdout/stderr is as simple as:
#
log-dir stdout
+# log-dir log
+
+# You can configure logcleanerday to control whether to enable logcleaner
+# and the maximum number of days that the INFO level logs will be kept.
+# if set logcleanerday to -1 , this mean disable logcleaner.
+# if set logcleanerday between 0 to INT_MAX , this mean enable logcleaner ,
+# and INFO level log file whose last modified time is greater than logcleanerday will be unlinked.
+# By default the logcleanerday is -1.
+logcleanerday -1
Review Comment:
yes ，but only INFO level will be unlinked。WARNING，ERROR，FATAL level log will be kept
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] IoCing commented on a diff in pull request #1171: add config about logcleaner
Posted by GitBox <gi...@apache.org>.
IoCing commented on code in PR #1171:
URL: https://github.com/apache/incubator-kvrocks/pull/1171#discussion_r1044071851
##########
src/config/config.cc:
##########
@@ -159,6 +159,7 @@ Config::Config() {
{"migrate-sequence-gap", false, new IntField(&sequence_gap, 10000, 1, INT_MAX)},
{"unixsocket", true, new StringField(&unixsocket, "")},
{"unixsocketperm", true, new OctalField(&unixsocketperm, 0777, 1, INT_MAX)},
+ {"logcleanerday", false, new IntField(&logcleanerday, -1, -1, INT_MAX)},
Review Comment:
thanks ，i get it
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] tisonkun merged pull request #1171: feat: add config for log cleaner
Posted by GitBox <gi...@apache.org>.
tisonkun merged PR #1171:
URL: https://github.com/apache/incubator-kvrocks/pull/1171
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] IoCing commented on a diff in pull request #1171: add config about logcleaner
Posted by GitBox <gi...@apache.org>.
IoCing commented on code in PR #1171:
URL: https://github.com/apache/incubator-kvrocks/pull/1171#discussion_r1044024764
##########
kvrocks.conf:
##########
@@ -96,6 +96,15 @@ dir /tmp/kvrocks
# We also can send logs to stdout/stderr is as simple as:
#
log-dir stdout
+# log-dir log
+
+# To enable the log cleaner. if set yes , glog will check if there are
+# overdue logs whenever a flush is performed
+enablelogcleaner yes
+
+# INFO level log file whose last modified time is greater than logcleanerday
+# will be unlinked
+logcleanerday 0
Review Comment:
thanks , i will add
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] git-hulk commented on a diff in pull request #1171: add config about logcleaner
Posted by GitBox <gi...@apache.org>.
git-hulk commented on code in PR #1171:
URL: https://github.com/apache/incubator-kvrocks/pull/1171#discussion_r1044119558
##########
src/config/config.cc:
##########
@@ -457,6 +458,15 @@ void Config::initFieldCallback() {
if (cluster_enabled) srv->slot_migrate_->SetSequenceGapSize(sequence_gap);
return Status::OK();
}},
+ {"log-retention-days",
+ [this](Server *srv, const std::string &k, const std::string &v) -> Status {
+ if (log_retention_days != -1) {
Review Comment:
```suggestion
[this](Server *srv, const std::string &k, const std::string &v) -> Status {
if (!srv) return Status::OK();
if (Util::ToLower(log_dir) == "stdout") {
return {Status::NotOK, "can't set the 'log-retention-days' when the log dir is stdout"};
}
if (log_retention_days != -1) {
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] torwig commented on a diff in pull request #1171: add config about logcleaner
Posted by GitBox <gi...@apache.org>.
torwig commented on code in PR #1171:
URL: https://github.com/apache/incubator-kvrocks/pull/1171#discussion_r1043546674
##########
kvrocks.conf:
##########
@@ -96,6 +96,15 @@ dir /tmp/kvrocks
# We also can send logs to stdout/stderr is as simple as:
#
log-dir stdout
+# log-dir log
+
+# To enable the log cleaner. if set yes , glog will check if there are
+# overdue logs whenever a flush is performed
+enablelogcleaner yes
+
+# INFO level log file whose last modified time is greater than logcleanerday
+# will be unlinked
+logcleanerday 0
Review Comment:
Could you please also mention in the comment what is the default value for `logcleanerday` and set it instead of 0? By the way, what will happen if the value of this setting is `0`? If 0 means "don't clean logs", you can mention it as well.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] git-hulk commented on pull request #1171: feat: add config for log cleaner
Posted by GitBox <gi...@apache.org>.
git-hulk commented on PR #1171:
URL: https://github.com/apache/incubator-kvrocks/pull/1171#issuecomment-1344091608
@IoCing Would you mind adding Go test cases for this configuration, to make sure it can be changed in-flight?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] torwig commented on a diff in pull request #1171: add config about logcleaner
Posted by GitBox <gi...@apache.org>.
torwig commented on code in PR #1171:
URL: https://github.com/apache/incubator-kvrocks/pull/1171#discussion_r1043546674
##########
kvrocks.conf:
##########
@@ -96,6 +96,15 @@ dir /tmp/kvrocks
# We also can send logs to stdout/stderr is as simple as:
#
log-dir stdout
+# log-dir log
+
+# To enable the log cleaner. if set yes , glog will check if there are
+# overdue logs whenever a flush is performed
+enablelogcleaner yes
+
+# INFO level log file whose last modified time is greater than logcleanerday
+# will be unlinked
+logcleanerday 0
Review Comment:
Could you please also mention in the comment what is the default value for `logcleanerday` and set it instead of 0? By the way, what will happen if the value of this setting is `0`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] git-hulk commented on a diff in pull request #1171: add config about logcleaner
Posted by GitBox <gi...@apache.org>.
git-hulk commented on code in PR #1171:
URL: https://github.com/apache/incubator-kvrocks/pull/1171#discussion_r1044116319
##########
kvrocks.conf:
##########
@@ -96,6 +96,16 @@ dir /tmp/kvrocks
# We also can send logs to stdout/stderr is as simple as:
#
log-dir stdout
+# log-dir log
+
+# You can configure log-retention-days to control whether to enable logcleaner
+# and the maximum number of days that the INFO level logs will be kept.
+# if set to -1 , this mean disable logcleaner.
+# if set to between 0 to INT_MAX , this mean enable logcleaner ,
+# and INFO level log file whose last modified time is greater than log-retention-days will be unlinked.
+# if set to 0 , during the flush, the previous INFO level logs will be immediately unlinked.
+# By default the log-retention-days is -1.
+log-retention-days -1
Review Comment:
```suggestion
# You can configure log-retention-days to control whether to enable the log cleaner
# and the maximum retention days that the INFO level logs will be kept.
#
# if set to -1, that means to disable the log cleaner.
# if set to 0, all previous INFO level logs will be immediately removed.
# if set to between 0 to INT_MAX, that means it will retent latest N(log-retention-days) day logs.
# By default the log-retention-days is -1.
log-retention-days -1
```
##########
src/config/config.cc:
##########
@@ -457,6 +458,15 @@ void Config::initFieldCallback() {
if (cluster_enabled) srv->slot_migrate_->SetSequenceGapSize(sequence_gap);
return Status::OK();
}},
+ {"log-retention-days",
+ [this](Server *srv, const std::string &k, const std::string &v) -> Status {
+ if (log_retention_days != -1) {
Review Comment:
```suggestion
[this](Server *srv, const std::string &k, const std::string &v) -> Status {
if (!srv) return Status::OK();
if (Util::ToLower(log_dir) == "stdout") {
return {Status::NotOK, "can't set the log-retention-days when the log dir is stdout"};
}
if (log_retention_days != -1) {
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] git-hulk commented on a diff in pull request #1171: add config about logcleaner
Posted by GitBox <gi...@apache.org>.
git-hulk commented on code in PR #1171:
URL: https://github.com/apache/incubator-kvrocks/pull/1171#discussion_r1044119558
##########
src/config/config.cc:
##########
@@ -457,6 +458,15 @@ void Config::initFieldCallback() {
if (cluster_enabled) srv->slot_migrate_->SetSequenceGapSize(sequence_gap);
return Status::OK();
}},
+ {"log-retention-days",
+ [this](Server *srv, const std::string &k, const std::string &v) -> Status {
+ if (log_retention_days != -1) {
Review Comment:
```suggestion
[this](Server *srv, const std::string &k, const std::string &v) -> Status {
if (!srv) return Status::OK();
if (Util::ToLower(log_dir) == "stdout") {
return {Status::NotOK, "can't set the log-retention-days when the log dir is 'stdout'"};
}
if (log_retention_days != -1) {
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] git-hulk commented on pull request #1171: add config about logcleaner
Posted by GitBox <gi...@apache.org>.
git-hulk commented on PR #1171:
URL: https://github.com/apache/incubator-kvrocks/pull/1171#issuecomment-1343896092
General looks good to me, except for two suggestions.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] tisonkun commented on a diff in pull request #1171: feat: add config for log cleaner
Posted by GitBox <gi...@apache.org>.
tisonkun commented on code in PR #1171:
URL: https://github.com/apache/incubator-kvrocks/pull/1171#discussion_r1044285449
##########
kvrocks.conf:
##########
@@ -97,6 +97,16 @@ dir /tmp/kvrocks
#
log-dir stdout
+# You can configure log-retention-days to control whether to enable the log cleaner
+# and the maximum retention days that the INFO level logs will be kept.
+#
+# if set to -1, that means to disable the log cleaner.
+# if set to 0, all previous INFO level logs will be immediately removed.
Review Comment:
I read we use the same config for all logs. How can logs in other levels have different manner?
##########
kvrocks.conf:
##########
@@ -97,6 +97,16 @@ dir /tmp/kvrocks
#
log-dir stdout
+# You can configure log-retention-days to control whether to enable the log cleaner
+# and the maximum retention days that the INFO level logs will be kept.
+#
+# if set to -1, that means to disable the log cleaner.
+# if set to 0, all previous INFO level logs will be immediately removed.
Review Comment:
I read we use the same config for all logs. How can logs in other levels have different manners?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] git-hulk commented on a diff in pull request #1171: feat: add config for log cleaner
Posted by GitBox <gi...@apache.org>.
git-hulk commented on code in PR #1171:
URL: https://github.com/apache/incubator-kvrocks/pull/1171#discussion_r1050483268
##########
kvrocks.conf:
##########
@@ -96,6 +96,15 @@ dir /tmp/kvrocks
# We also can send logs to stdout/stderr is as simple as:
#
log-dir stdout
+# log-dir log
+
+# You can configure logcleanerday to control whether to enable logcleaner
+# and the maximum number of days that the INFO level logs will be kept.
+# if set logcleanerday to -1 , this mean disable logcleaner.
+# if set logcleanerday between 0 to INT_MAX , this mean enable logcleaner ,
+# and INFO level log file whose last modified time is greater than logcleanerday will be unlinked.
+# By default the logcleanerday is -1.
+logcleanerday -1
Review Comment:
Thanks for your re-claimed, it'd be better if it could remove all level logs instead of only the INFO level.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] tisonkun commented on pull request #1171: feat: add config for log cleaner
Posted by GitBox <gi...@apache.org>.
tisonkun commented on PR #1171:
URL: https://github.com/apache/incubator-kvrocks/pull/1171#issuecomment-1361157912
It seems Travis CI takes a longer time to finish. Perhaps we should restore the GitHub Actions + QEMU solution XD.
Merging...
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org