feat: add CLIENT SETNAME/SETINFO support - #2790
Open
nesty92 wants to merge 1 commit into
Open
Conversation
Copy link
Copy Markdown
Sorry, something went wrong.
Copy link
Copy Markdown
Member
|
Thanks! I like the idea in general. The only concerns I have are:
|
Sorry, something went wrong.
Improve connection visibility and debugging. Setting client names allows operators to identify which application a connection belongs to using CLIENT LIST. CLIENT SETINFO follows Redis best practices for client libraries, providing library name and version metadata for monitoring. Signed-off-by: Ernesto Alejandro Santana Hidalgo <ernesto.alejandrosantana@gmail.com>
Copy link
Copy Markdown
Author
|
Hi @michael-grunder, thanks for the feedback, having this feature for short time connections is not useful so we could skip those. Let me know your thoughts about it +-----------------------+
| START HANDSHAKE |
+-----------+-----------+
|
v
Is Connection Persistent?
|
NO ---+--- YES
| |
| v
| +-----------------------------------------------+
| | Build & Send "HELLO 2" Command |
| | (Includes AUTH & SETNAME as arguments) |
| +-----------------------+-----------------------+
| |
| v
| Response is okay?
| |
| NO ---------+--------- YES
| | |
| v v
| +---------------------+ +---------------------+
| | FALLBACK | | REDIS 6+ |
|-> | (Legacy Logic) | | (Modern Logic) |
+----------+----------+ +----------+----------+
| |
v v
Has Password? Parse Server Info
/ \ (server, version)
NO YES |
| | |
| v |
| Send AUTH cmd |
SUCCESS <---+-- OK? <-------+ |
^ | | |
| | NO --> FAILURE |
| | |
| v |
| Has Client Name? |
| / \ |
| NO YES |
| | | |
| | v |
+--+ Send CLIENT SETNAME |
| | |
| v |
+------- OK? <---+ |
/ \ |
NO YES |
| \ |
| \ v
FAILURE SUCCESS
|
Sorry, something went wrong.
Copy link
Copy Markdown
Member
|
Yeah a lot of that makes sense. Persistent connections can be expected to live longer. The absolute safest choice would be to gate it behind an option, but I'll play around with it over the weekend. |
Sorry, something went wrong.
Copy link
Copy Markdown
Author
|
Hi @michael-grunder any news on this one?? |
Sorry, something went wrong.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
While it's possible to set the client name with the current api from php, it will set the client name on each execution from the php side, moving it to the lib in self could make it more reliable and easy to use.
A new status for the connections, REUSED will help to track the lifecycle of the connection, the new HELLO could be useful to implement RESP 3 or Availability Zone affinity for Valkey
CLIENT SETINFO follows Redis best practices for client libraries, providing library name and version metadata for monitoring.