-
Notifications
You must be signed in to change notification settings - Fork 698
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
Add availability_zone
to the HELLO response
#1487
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the idea but i think we should only include it if it's configured.
Signed-off-by: Rueian <rueiancsie@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@valkey-io/core-team please approve if you agree.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, please also add a test, i think it can be fit in protocol.tcl
Signed-off-by: Rueian <rueiancsie@gmail.com>
Thanks for the direction but how about we add the test in the tracking.tcl? That is the only place I found that has tests for the HELLO command. |
tracking.tcl seem odd, the test does not do anything close to tracking, i suggest we also move |
Signed-off-by: Rueian <rueiancsie@gmail.com>
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## unstable #1487 +/- ##
============================================
- Coverage 70.86% 70.85% -0.02%
============================================
Files 119 119
Lines 64859 64863 +4
============================================
- Hits 45963 45959 -4
- Misses 18896 18904 +8
|
Updated. |
Signed-off-by: Rueian <rueiancsie@gmail.com>
This PR is for valkey-io/valkey#1487. Signed-off-by: Rueian <rueiancsie@gmail.com>
Thanks for all the reviewing. Here is the PR for doc changes: valkey-io/valkey-doc#207. |
This PR is a followup for #1487. Signed-off-by: Rueian <rueiancsie@gmail.com> Co-authored-by: Binbin <binloveplay1314@qq.com>
Documentation for valkey-io/valkey#1487. It includes the `availability_zone` field in the examples and has a note saying that the field will only be present when it is set in configs. Signed-off-by: Rueian <rueiancsie@gmail.com>
It's inconvenient for client implementations to extract the
availability_zone
information from theINFO
response. TheINFO
response contains a lot of information that a client implementation typically doesn't need.This PR adds the availability zone to the
HELLO
response. Clients usually already use theHELLO
command for protocol negotiation and also get the serverversion
androle
from its response. To keep theHELLO
response small, the field is only added if availability zone is configured.