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

Add RESP2 to RESP3 replies migration guide #2513

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

uglide
Copy link
Contributor

@uglide uglide commented Aug 11, 2023

See #2511 for more details.

@netlify
Copy link

netlify bot commented Aug 11, 2023

👷 Deploy request for redis-doc pending review.

Visit the deploys page to approve it

Name Link
🔨 Latest commit e68960e

docs/reference/resp2-to-resp3-replies.md Outdated Show resolved Hide resolved
docs/reference/resp2-to-resp3-replies.md Outdated Show resolved Hide resolved
docs/reference/resp2-to-resp3-replies.md Outdated Show resolved Hide resolved
@mgravell
Copy link
Contributor

I believe there may be some more "verbatim string" swaps; INFO seems to use this; might be worth a source scan; LATENCY DOCTOR and LOLWUT? (I haven't checked those)

@slorello89
Copy link
Member

slorello89 commented Aug 16, 2023

To @mgravell's point - CLUSTER INFO, CLUSTER NODES, MEMORY DOCTOR all are verbatim in RESP3.

@uglide
Copy link
Contributor Author

uglide commented Sep 1, 2023

@mgravell @slorello89 @oranagra Sorry for the delay. The document was updated.

Copy link
Member

@slorello89 slorello89 left a comment

Choose a reason for hiding this comment

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

I did a pretty thorough review of the RESP3 v RESP2 command results for OSS on my own - this list seems comprehensive to me.

NickCraver added a commit to StackExchange/StackExchange.Redis that referenced this pull request Sep 7, 2023
Overall changes:
- [x] introduce `Resp2Type` and `Resp3Type` shims (`Resp2Type` has reduced types); existing code using `[Obsolete] Type` uses `Resp2Type` for minimal code impact
- [x] mark existing `Type` as `[Obsolete]`, and proxy to `Resp2Type` for compat
- [x] deal with null handling differences
- [x] deal with `Boolean`, which works very differently (`t`/`f` instead of `1`/`0`)
- [x] deal with `[+|-]{inf|nan}` when parsing doubles (explicitly called out in the RESP3 spec)
- [x] parse new tokens
- [x] `HELLO` handshake
  - [x] core message and result handling
  - [x] validation and fallback
- [x] prove all return types (see: redis/redis-specifications#15)
- [x] <strike>streamed RESP3</strike> omitting; not implemented by server; can revisit
- [x] deal with pub/sub differences
  - [x] check routing of publish etc
  - [x] check re-wire of subscription if failed
  - [x] check receive notifications
  - [x] connection management (i.e. not to spin up in resp3)
  - [x] connection fallback spinup (i.e. if we were trying resp3 but failed)
- [x] other
  - [x] [undocumented RESP3 delta](redis/redis-doc#2513)
  - [x] run core tests in both RESP2 and RESP3
  - [x] compensate for tests that expect separate subscription connections

Co-authored-by: Nick Craver <[email protected]>
Co-authored-by: Nick Craver <[email protected]>
Copy link
Collaborator

@dwdougherty dwdougherty left a comment

Choose a reason for hiding this comment

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

Some copy-edits and style guide updates.

I also suggest using the same type names that are found on the protocol page. For example, change "[bB]lob[sS]tring" to "bulk string" (?) as appropriate.

docs/reference/resp2-to-resp3-replies.md Outdated Show resolved Hide resolved
docs/reference/resp2-to-resp3-replies.md Outdated Show resolved Hide resolved
docs/reference/resp2-to-resp3-replies.md Outdated Show resolved Hide resolved
docs/reference/resp2-to-resp3-replies.md Outdated Show resolved Hide resolved
docs/reference/resp2-to-resp3-replies.md Outdated Show resolved Hide resolved
docs/reference/resp2-to-resp3-replies.md Outdated Show resolved Hide resolved
docs/reference/resp2-to-resp3-replies.md Outdated Show resolved Hide resolved
docs/reference/resp2-to-resp3-replies.md Outdated Show resolved Hide resolved
@CLAassistant
Copy link

CLAassistant commented Mar 21, 2024

CLA assistant check
All committers have signed the CLA.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants