-
Notifications
You must be signed in to change notification settings - Fork 632
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
Rename redis in aof logs and proc title redis-aof-rewrite to valkey-aof-rewrite. #393
Rename redis in aof logs and proc title redis-aof-rewrite to valkey-aof-rewrite. #393
Conversation
Signed-off-by: Shivshankar-Reddy <shiva.sheri.github@gmail.com>
TSC had a discussion on this topic (#361) The rule of thumb is that command error messages should be retained for backward compatibility. Log lines are in general OK to change. We haven't discussed the proc title; though I consider it much closer to logs (hence safe to change). @zuiderkwast thoughts? |
src/aof.c
Outdated
@@ -996,7 +996,7 @@ int startAppendOnly(void) { | |||
|
|||
if (rewriteAppendOnlyFileBackground() == C_ERR) { | |||
server.aof_state = AOF_OFF; | |||
serverLog(LL_WARNING,"Redis needs to enable the AOF but can't trigger a background AOF rewrite operation. Check the above logs for more info about the error."); | |||
serverLog(LL_WARNING,"Server needs to enable the AOF but can't trigger a background AOF rewrite operation. Check the above logs for more info about the error."); |
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 is part of #207 so I will tag @0del because he is assigned to it. @0del are you still working on the remaining logging?
In #207 we discussed this style:
serverLog(LL_WARNING,"%s needs to enable the AOF but can't trigger a background AOF rewrite operation. Check the above logs for more info about the error.",
SERVER_TITLE);
... because then it would be easy patch the SERVER_TITLE macro to be "Redis" to get the old logging. But then we discussed it a bit more and decided that most of the logging isn't a problem about redis compatibility, so I think we don't need this complexity.
I'm OK with "Server", but probably better grammar is "The server" in this case.
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.
Thank you for the review, Changed the string "Server" to "The server" and updated the diff.
src/aof.c
Outdated
@@ -2471,7 +2471,7 @@ int rewriteAppendOnlyFileBackground(void) { | |||
char tmpfile[256]; | |||
|
|||
/* Child */ | |||
serverSetProcTitle("redis-aof-rewrite"); | |||
serverSetProcTitle("valkey-aof-rewrite"); |
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.
@PingXie You're right that proc title is not logging.
For the most reasonable result we could check if Valkey was started using a redis-server
symlink, similar to how we check if we was started as valkey-check-aof
and other names, here:
Line 7019 in a5a1377
if (strstr(exec_name,"valkey-check-rdb") != NULL) |
Maybe it's too complex and not important. I'm not sure.
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.
check if Valkey was started using a redis-server symlink
+1
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 have tried to add a flag similar to server.sentinel_mode, I am still not sure whether it is appropriate and best way.
Signed-off-by: Shivshankar-Reddy <shiva.sheri.github@gmail.com>
Signed-off-by: Shivshankar-Reddy <shiva.sheri.github@gmail.com>
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## unstable #393 +/- ##
============================================
+ Coverage 68.35% 68.42% +0.06%
============================================
Files 108 109 +1
Lines 61563 61671 +108
============================================
+ Hits 42084 42198 +114
+ Misses 19479 19473 -6
|
Signed-off-by: Shivshankar-Reddy <shiva.sheri.github@gmail.com>
Signed-off-by: Shivshankar-Reddy <shiva.sheri.github@gmail.com>
Signed-off-by: Shivshankar-Reddy <shiva.sheri.github@gmail.com>
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.
Good, but we don't need to store that bool.
Signed-off-by: Shivshankar-Reddy <shiva.sheri.github@gmail.com>
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.
Looks good!
…of-rewrite (valkey-io#393) Renamed redis to valkey/server in aof.c serverlogs. The AOF rewrite child process title is set to "redis-aof-rewrite" if Valkey was started from a redis-server symlink, otherwise to "valkey-aof-rewrite". This is a breaking changes since logs are changed. Part of valkey-io#207. --------- Signed-off-by: Shivshankar-Reddy <shiva.sheri.github@gmail.com>
Renamed redis to valkey/server in aof.c serverlogs.
The AOF rewrite child process title is set to "redis-aof-rewrite" if Valkey was started from a redis-server symlink, otherwise to "valkey-aof-rewrite".
This is a breaking changes since logs are changed.
Part of #207.