Skip to content

Remove 'Redis' in error replies#206

Merged
zuiderkwast merged 5 commits into
valkey-io:unstablefrom
zuiderkwast:redis-in-error-replies
Apr 16, 2024
Merged

Remove 'Redis' in error replies#206
zuiderkwast merged 5 commits into
valkey-io:unstablefrom
zuiderkwast:redis-in-error-replies

Conversation

@zuiderkwast

@zuiderkwast zuiderkwast commented Apr 4, 2024 •
edited
Loading

Copy link
Copy Markdown
Contributor

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()"

@zuiderkwast
Remove 'Redis' in error replies
405367d 
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

zuiderkwast commented Apr 4, 2024

Copy link
Copy Markdown
Contributor Author

@0del do you want to review this one?

@zuiderkwast
Fix testcase
4ac8a77 
Signed-off-by: Viktor Söderqvist <viktor.soderqvist@est.tech>
@zuiderkwast zuiderkwast force-pushed the redis-in-error-replies branch from ca07ce1 to 4ac8a77 Compare April 4, 2024 16:33
9bany
9bany reviewed Apr 4, 2024
View reviewed changes
Comment thread tests/unit/acl.tcl
9bany
9bany approved these changes Apr 4, 2024
View reviewed changes

@9bany 9bany left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM !

@hwware hwware mentioned this pull request Apr 4, 2024
Closed
madolson
madolson reviewed Apr 4, 2024
View reviewed changes
Comment thread src/server.c Outdated
@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

PingXie commented Apr 5, 2024

Copy link
Copy Markdown
Member

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 mentioned this pull request Apr 9, 2024
Closed
@zuiderkwast
Merge remote-tracking branch 'valkey/unstable' into redis-in-error-re…
1da8c5a 
…plies
@zuiderkwast
Revert 'high risk' error replies
5d04451 
Signed-off-by: Viktor Söderqvist <viktor.soderqvist@est.tech>
@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

zuiderkwast commented Apr 12, 2024

Copy link
Copy Markdown
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
@zuiderkwast
'the Valkey source' => 'the source'
7b73d8d 
Signed-off-by: Viktor Söderqvist <viktor.soderqvist@est.tech>
@zuiderkwast zuiderkwast force-pushed the redis-in-error-replies branch from b169043 to 7b73d8d Compare April 15, 2024 22:29
@zuiderkwast zuiderkwast merged commit 8dcc8eb into valkey-io:unstable Apr 16, 2024
@zuiderkwast zuiderkwast deleted the redis-in-error-replies branch April 16, 2024 19:17
zuiderkwast added a commit to zuiderkwast/valkey that referenced this pull request Apr 17, 2024
@zuiderkwast
Remove 'Redis' in error replies (valkey-io#206)
31a79cd 
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
@zuiderkwast @PatrickJS
Remove 'Redis' in error replies (valkey-io#206)
fdcd4b5 
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>
bjosv added a commit to bjosv/valkey that referenced this pull request Aug 20, 2025
@bjosv
Update deps/libvalkey to version 0.2.1
c140cb9 
Squashed 'deps/libvalkey/' changes from abcd27fbf..d95a5ee9a

9e10acbf7 Fix duplicate Acks for RDMA events. (valkey-io#229)
1eadedf48 Remove the unused valDup API from dict
a449f0ea1 Don't expose internal functions in shared libraries (valkey-io#205)
178e350c7 Cluster code cleanup (valkey-io#216)
4020396c8 Fix `unused-parameter` warning when building with `NDEBUG` (valkey-io#212)
279125e22 Spelling (valkey-io#213)
99aa158bc Use existing connections for blocking slotmap updates (valkey-io#199)
f670346b9 Only install headers for `TLS` and `RDMA` when enabled in CMake (valkey-io#206)
41c5911f1 Remove macro UNUSED from public API (valkey-io#200)
969a8c546 Fix dependency issue with RDMA (valkey-io#201)

git-subtree-dir: deps/libvalkey
git-subtree-split: d95a5ee9ad95b9ee7f298f38d0fb93f3f7659ef7

Signed-off-by: Björn Svensson <bjorn.a.svensson@est.tech>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@madolson madolson madolson left review comments
@enjoy-binbin enjoy-binbin enjoy-binbin approved these changes
+2 more reviewers
@9bany 9bany 9bany approved these changes
@hwware hwware hwware approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

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

Valkey 8.0
Status: Done

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Occurrences of "Redis" in error replies

6 participants

@zuiderkwast @PingXie @enjoy-binbin @madolson @hwware @9bany