Skip to content
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

Remove 'Redis' in error replies #206

Merged
merged 5 commits into from
Apr 16, 2024

Conversation

zuiderkwast
Copy link
Contributor

@zuiderkwast zuiderkwast commented Apr 4, 2024

Low-risk error replies containing "Redis" are changed, excluding the following ones that are instead covered in #274 (since they're high risk of causing problems):

  • (not in scope) "-MISCONF Redis is configured to save RDB snapshots (...)"
  • (not in scope) "-LOADING Redis is loading the dataset in memory"
  • (not in scope) "-BUSY Redis is busy running a script (...)"

Additionally, error replies from redis.call in a Lua script are affected, such as

  • "Please specify at least one argument for this redis lib call"
  • "Wrong number of args calling Redis command from script"
  • "Unknown Redis command called from script"
  • "Invalid command passed to redis.acl_check_cmd()"

The name Redis is simply removed from these error message. In the last one above, "redis.acl_check_cmd()" is replaced by "server.acl_check_cmd()" in the error message.

Fixes #204

For release notes:

The word "Redis" is removed in the following error replies:

  • "This Redis instance is not configured to use an ACL file. You may want to specify users via the ACL SETUSER command and then issue a CONFIG REWRITE (assuming you have a Redis configuration file set) in order to store users in the Redis configuration."
  • "Active defragmentation cannot be enabled: it requires a Redis server compiled with a modified Jemalloc like the one shipped by default with the Redis source requires a server compiled with a modified Jemalloc like the one shipped by default with the Valkey source distribution"
  • "Wrong number of args calling Redis command from script"
  • "Unknown Redis command called from script"

The words "used by Redis" are removed in the error message

  • "The event loop API used by Redis is not able to handle the specified number of clients"

The words "since Redis 4" are removed in the error message

  • "WAIT cannot be used with replica instances. Please also note that since Redis 4.0 if a replica is configured to be writable (which is not the default) writes to replicas are just local and are not propagated."

The words "redis lib" are removed from in the error message

  • "Please specify at least one argument for this redis lib call"

"Lua redis lib command" replaced with just "Command" in the error message

  • "Lua redis lib command arguments must be strings or integers"

"redis.acl_check_cmd" replaced by "server.acl_check_cmd" in the following error message:

  • "Invalid command passed to redis.acl_check_cmd()"

This includes error message like

* "-MISCONF Redis is configured to save RDB snapshots (...)"
* "-LOADING Redis is loading the dataset in memory"
* "-BUSY Redis is busy running a script (...)"

but also error replies from `redis.call` in a Lua script, such as

* "Please specify at least one argument for this redis lib call"
* "Wrong number of args calling Redis command from script"
* "Unknown Redis command called from script"
* "Invalid command passed to redis.acl_check_cmd()"

The name Redis is simply removed from these error message.
In the last one above, "redis.acl_check_cmd()" is replaced by
"server.acl_check_cmd()" in the error message.

Signed-off-by: Viktor Söderqvist <viktor.soderqvist@est.tech>
@zuiderkwast
Copy link
Contributor Author

@0del do you want to review this one?

Signed-off-by: Viktor Söderqvist <viktor.soderqvist@est.tech>
Copy link
Contributor

@9bany 9bany left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM !

src/server.c Outdated Show resolved Hide resolved
@zuiderkwast zuiderkwast added major-decision-pending Major decision pending by TSC team breaking-change Indicates a possible backwards incompatible change labels Apr 4, 2024
@PingXie
Copy link
Member

PingXie commented Apr 5, 2024

I don't think we should make this change. We should use the indirection mechanism established in #47 instead. As we discussed in other threads, I am proposing to have one uber compilation flag such as '-DREDIS_COMAPT=1' to control the presence/absence of all breaking changes.

@zuiderkwast zuiderkwast added major-decision-approved Major decision approved by TSC team and removed major-decision-pending Major decision pending by TSC team labels Apr 12, 2024
@zuiderkwast
Copy link
Contributor Author

@valkey-io/core-team After decision made about #274, I've reduced the scope of this PR to only include "low risk" error replies. The "high risk" ones are handled in #274 instead.

I consider the scope of this PR (and the linked issue) already decided. It just needs a second review before merge.

@zuiderkwast zuiderkwast added the release-notes This issue should get a line item in the release notes label Apr 15, 2024
Signed-off-by: Viktor Söderqvist <viktor.soderqvist@est.tech>
@zuiderkwast zuiderkwast merged commit 8dcc8eb into valkey-io:unstable Apr 16, 2024
14 checks passed
@zuiderkwast zuiderkwast deleted the redis-in-error-replies branch April 16, 2024 19:17
zuiderkwast added a commit to zuiderkwast/placeholderkv that referenced this pull request Apr 17, 2024
Low-risk error replies containing "Redis" are changed.

In most cases, the word "Redis" is simply removed from the error message,
such as in "This Redis instance is not configured to use an ACL file. (...)",
the message is changed to "This instance is not configured to use an ACL
file. (...)".

Additionally, error replies from `redis.call` in a Lua script are
affected, such as

* "Please specify at least one argument for this redis lib call"
* "Wrong number of args calling Redis command from script"
* "Unknown Redis command called from script"
* "Invalid command passed to redis.acl_check_cmd()"

The name Redis is simply removed from these error message. In the last
one above, "redis.acl_check_cmd()" is replaced by
"server.acl_check_cmd()" in the error message.

The following error replies are considered high of causing problems for
clients, so they are not changed in this commit:

* (not in scope) "-MISCONF Redis is configured to save RDB snapshots
(...)"
* (not in scope) "-LOADING Redis is loading the dataset in memory"
* (not in scope) "-BUSY Redis is busy running a script (...)"

Fixes valkey-io#204

---------

Signed-off-by: Viktor Söderqvist <viktor.soderqvist@est.tech>
PatrickJS pushed a commit to PatrickJS/placeholderkv that referenced this pull request Apr 24, 2024
Low-risk error replies containing "Redis" are changed.

In most cases, the word "Redis" is simply removed from the error message,
such as in "This Redis instance is not configured to use an ACL file. (...)",
the message is changed to "This instance is not configured to use an ACL
file. (...)".

Additionally, error replies from `redis.call` in a Lua script are
affected, such as

* "Please specify at least one argument for this redis lib call"
* "Wrong number of args calling Redis command from script"
* "Unknown Redis command called from script"
* "Invalid command passed to redis.acl_check_cmd()"

The name Redis is simply removed from these error message. In the last
one above, "redis.acl_check_cmd()" is replaced by
"server.acl_check_cmd()" in the error message.

The following error replies are considered high of causing problems for
clients, so they are not changed in this commit:

* (not in scope) "-MISCONF Redis is configured to save RDB snapshots
(...)"
* (not in scope) "-LOADING Redis is loading the dataset in memory"
* (not in scope) "-BUSY Redis is busy running a script (...)"

Fixes valkey-io#204

---------

Signed-off-by: Viktor Söderqvist <viktor.soderqvist@est.tech>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking-change Indicates a possible backwards incompatible change major-decision-approved Major decision approved by TSC team release-notes This issue should get a line item in the release notes
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Occurrences of "Redis" in error replies
6 participants