Change Redis to Valkey in log messages#226
Conversation
Signed-off-by: 0del <bany.y0599@gmail.com>
|
a smaller PR 😄 @zuiderkwast |
Signed-off-by: 0del <bany.y0599@gmail.com>
|
Let's use the indirection (via SERVER_NAME) set up by #47 so we can easily build a binary that logs in the exact same way. |
|
@PingXie i see, let me make some changes |
zuiderkwast
left a comment
There was a problem hiding this comment.
We'll probably want this to be controlled by a compatibility config, to be as compatible as possible with redis in terms of script parsing the logs, etc. We'll not do this in our first release anyway.
Please leave this PR open for discussion so we can decide later.
|
Related: #43 |
Closed the pr? Don't want to leave it open? |
|
Sorry, i forget to open a issue |
|
I'm going to open this again. |
|
What is the problem needed to change in this PR @zuiderkwast ? |
Nothing. I just want it to be open and waiting. :) I wrote earlier "Please leave this PR open for discussion so we can decide later." I want this PR but I am not a dictator of valkey. The other maintainers need to decide about it too. But right now we are busy with our first release (7.2.4) and this change is better for the next release (8.0). |
zuiderkwast
left a comment
There was a problem hiding this comment.
Don't merge. See #207.
| @@ -1,4 +1,5 @@ | |||
| #define SERVER_NAME "valkey" | |||
| #define SERVER_TITLE "Valkey" | |||
There was a problem hiding this comment.
Was the reason to make this SERVER_TITLE to make it independent from SERVER_NAME so we could add the compatibility switch later?
There was a problem hiding this comment.
It's "Valkey" vs "valkey". It was discussed elsewhere (some clicks away).
@0del please update the PR title and write a description about the change, including what SERVER_TITLE is.
There was a problem hiding this comment.
And yes, also for the compat switch.
There was a problem hiding this comment.
Maybe call it SERVER_NAME_LOWER then? At first glance I didn't notice the difference
There was a problem hiding this comment.
TITLE is in "title case", i.e. first letter capitalized.
(It was my suggestion.)
| serverLog(LL_WARNING, | ||
| "Configuring Redis with users defined in redis.conf and at " | ||
| "Configuring %s with users defined in valkey.conf and at " |
There was a problem hiding this comment.
The purpose of having %s here is to be keep to the log messages exactly as they were in Redis, because someone may search for exactly this string in the logs. If we want to support that, we must also keep "redis.conf" instead of "valkey.conf". Otherwise the string is still changed.
Personally, I think it makes no have the special compatibility mode for a log message like this one.
@PingXie FYI
There was a problem hiding this comment.
It's a little bit strange that with the macro, it's possible for a user to patch the macro, like #define SERVER_TITLE "Redis", to make this message say "Redis" but it will still contain "valkey.conf" so the message is still not identical to in Redis. But we decided that we don't need to be 100% compatible in these log messages, I'm going to ignore that. We need to finish this rebranding some time. :)
zuiderkwast
left a comment
There was a problem hiding this comment.
I'm fine with these changes. We've decided that log messages don't need to be identical to Redis log messages (except the markers for the crash report).
Signed-off-by: 0del <bany.y0599@gmail.com>
Log messages containing "Redis" in some files are changed. Add macro SERVER_TITLE defined to "Valkey" (uppercase V) is introduced and used in log messages, so at least it will be easy to patch this definition to get Redis or any other name in the logs instead of Valkey. Change "Redis" in some log messages to use %s and SERVER_TITLE. This is a partial implementation of valkey-io#207 --------- Signed-off-by: 0del <bany.y0599@gmail.com>
Log messages containing "Redis" in some files are changed. Add macro SERVER_TITLE defined to "Valkey" (uppercase V) is introduced and used in log messages, so at least it will be easy to patch this definition to get Redis or any other name in the logs instead of Valkey. Change "Redis" in some log messages to use %s and SERVER_TITLE. This is a partial implementation of valkey-io#207 --------- Signed-off-by: 0del <bany.y0599@gmail.com>
Log messages containing "Redis" in some files are changed.
Add macro SERVER_TITLE defined to "Valkey" (uppercase V) is introduced and used in log messages, so at least it will be easy to patch this definition to get Redis or any other name in the logs instead of Valkey.
Change "Redis" in some log messages to use %s and SERVER_TITLE.
This is a partial implementation of #207