valkey
valkey copied to clipboard
valkey-io
rename redis to valkey in serverlog
Let's not merge this change just yet. We should agree on some guiding rules and make sure all log line changes are evaluated against these rules consistently. Ad hoc decisions should be avoided.
As I mentioned in other threads already, I would suggest having all these "Redis" references done through the indirection of "SERVER_NAME", at lease for any log levels above DEBUG, not inclusive.
The only plus side of dropping these redis references is that the log line looks cleaner, aesthetically, but the downside is hard to quantify, especially for users who self-manages their own instances. It is going to be quite annoying for these users to have to update their home-grown log parsers. This creates unnecessary adoption barriers IMO.
@PingXie It's a good idea to use a macro like SERVER_NAME. If we are careful, it will be easy to patch valkey to return the exact Redis log messages by just changing this define.
SERVER_NAME is "valkey", so I think we should have another one like SERVER_TITLE for "Valkey" (with capital V).
Ping, can we go ahead and do it in this way? (not yet touching the log entries mentioned in #274)
@PingXie It's a good idea to use a macro like SERVER_NAME. If we are careful, it will be easy to patch valkey to return the exact Redis log messages by just changing this define.
SERVER_NAME is "valkey", so I think we should have another one like SERVER_TITLE for "Valkey" (with capital V).
Ping, can we go ahead and do it in this way? (not yet touching the log entries mentioned in #274)
Yeah - that will be great. Let's create the indirection first and we can discuss the "switch". I can see either compile-time switch or run-time switch works, as long as we have the indirection in place.
I like the "title" idea too. We should document in the code the guidance on when to use which: "SERVER_NAME" vs "SERVER_TITLE" as well.
@PingXie If we use a macro like ...
serverLog(LL_NOTICE, SERVER_TITLE " forked for debugging eval");
... then it only works in compile time. If we want runtime, we need to rewrite it like ...
serverLog(LL_NOTICE, "%s forked for debugging eval", SERVER_TITLE);
IMO, we don't need a runtime switch (config) for all of these log entries. We only need it for a few of them, the ones mentioned in the redis-compatility issue. (That one is also more complext, e.g. it controls what we show as "version" in HELLO, etc.)
WDYT, can we treat it as a compile-time in low-risk cases like these lua debugger ones?
@PingXie If we use a macro like ...
serverLog(LL_NOTICE, SERVER_TITLE " forked for debugging eval");... then it only works in compile time. If we want runtime, we need to rewrite it like ...
serverLog(LL_NOTICE, "%s forked for debugging eval", SERVER_TITLE);IMO, we don't need a runtime switch (config) for all of these log entries. We only need it for a few of them, the ones mentioned in the redis-compatility issue. (That one is also more complext, e.g. it controls what we show as "version" in HELLO, etc.)
WDYT, can we treat it as a compile-time in low-risk cases like these lua debugger ones?
Right, we would need to use %s to keep the door open.
I don't know if we have a consensus on compile-time vs run-time. I can see run-time gives users more flexibility while the image distributors have few decisions to make (should I package the Redis one or Valkey one? or both? how should I name them if both are provided?). So it would be good if we could decouple that discussion from this indirection change.