Skip to content
This repository was archived by the owner on Mar 9, 2026. It is now read-only.

Add RESP2 to RESP3 replies migration guide#2513

Open
uglide wants to merge 5 commits intoredis:masterfrom
uglide:resp2-to-resp3-migration-guide
Open

Add RESP2 to RESP3 replies migration guide#2513
uglide wants to merge 5 commits intoredis:masterfrom
uglide:resp2-to-resp3-migration-guide

Conversation

@uglide
Copy link
Copy Markdown
Contributor

@uglide uglide commented Aug 11, 2023

See #2511 for more details.

@netlify
Copy link
Copy Markdown

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

@mgravell
Copy link
Copy Markdown
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
Copy Markdown
Member

slorello89 commented Aug 16, 2023

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

@uglide
Copy link
Copy Markdown
Contributor Author

uglide commented Sep 1, 2023

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

Copy link
Copy Markdown
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 <nickcraver@microsoft.com>
Co-authored-by: Nick Craver <nrcraver@gmail.com>
Copy link
Copy Markdown
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.

Co-authored-by: David Dougherty <david.dougherty@redis.com>
@CLAassistant
Copy link
Copy Markdown

CLAassistant commented Mar 21, 2024

CLA assistant check
All committers have signed the CLA.

@mgravell
Copy link
Copy Markdown
Contributor

just wondering... any plans to merge this ever?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants