Skip to content

Conversation

@zuiderkwast
Copy link
Contributor

Fixes #11420.

Copy link
Contributor

@enjoy-binbin enjoy-binbin left a comment

Choose a reason for hiding this comment

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

we also need to run the generate-command-code.py

Copy link
Member

@oranagra oranagra left a comment

Choose a reason for hiding this comment

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

LGTM

"arity": -1,
"function": "quitCommand",
"deprecated_since": "7.2.0",
"replaced_by": "just closing the connection",
Copy link
Member

Choose a reason for hiding this comment

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

@guybe7 did we have any plans for replaced_by to always point to a specific command (rather than just plain text)?

Copy link
Collaborator

Choose a reason for hiding this comment

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

we don't rely on it, but i'd rather replaced_by would always be a command name

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Currently, it is a string of text usually with some commands embedded within it. I think we should follow that until we change it for all commands. Some examples:

georadius_ro.json:        "replaced_by": "`GEOSEARCH` with the `BYRADIUS` argument",
georadiusbymember.json:        "replaced_by": "`GEOSEARCH` and `GEOSEARCHSTORE` with the `BYRADIUS` and `FROMMEMBER` arguments",
georadiusbymember_ro.json:        "replaced_by": "`GEOSEARCH` with the `BYRADIUS` and `FROMMEMBER` arguments",

On the redis.io website, it is rendered inside a sentence so we need to take care that the grammar ends up correct:

As of Redis version {deprecated_since}, this command is regarded as deprecated.

It can be replaced by {replaced_by} when migrating or writing new code.

Copy link
Member

@oranagra oranagra left a comment

Choose a reason for hiding this comment

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

approved in a core-team meeting for 7.2.
@zuiderkwast can you make a PR for redis-docs with the content Yossi mentioned in #11420

@oranagra oranagra added state:needs-doc-pr requires a PR to redis-doc repository release-notes indication that this issue needs to be mentioned in the release notes labels Nov 9, 2022
@zuiderkwast
Copy link
Contributor Author

can you make a PR for redis-docs

Done. I'm not sure how much detail to include in the docs apart from "don't use it", so feel free to edit the docs it before doc merge.

@oranagra oranagra added the state:major-decision Requires core team consensus label Nov 9, 2022
@oranagra oranagra merged commit 07d1870 into redis:unstable Nov 9, 2022
@zuiderkwast zuiderkwast deleted the deprecate-quit branch November 9, 2022 15:01
madolson pushed a commit to madolson/redis that referenced this pull request Apr 19, 2023
Clients should not use this command.
Instead, clients should simply close the connection when they're not used anymore.
Terminating a connection on the client side is preferable, as it eliminates `TIME_WAIT`
lingering sockets on the server side.
enjoy-binbin pushed a commit to enjoy-binbin/redis that referenced this pull request Jul 31, 2023
Clients should not use this command.
Instead, clients should simply close the connection when they're not used anymore.
Terminating a connection on the client side is preferable, as it eliminates `TIME_WAIT`
lingering sockets on the server side.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

release-notes indication that this issue needs to be mentioned in the release notes state:major-decision Requires core team consensus state:needs-doc-pr requires a PR to redis-doc repository

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

Deprecate QUIT

4 participants