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 support of RESP3 verbatim string #2102

Merged
merged 8 commits into from
Feb 18, 2024
Merged

Add support of RESP3 verbatim string #2102

merged 8 commits into from
Feb 18, 2024

Conversation

wsehjk
Copy link
Contributor

@wsehjk wsehjk commented Feb 14, 2024

This pr is to close #2036.

@wsehjk
Copy link
Contributor Author

wsehjk commented Feb 14, 2024

This implementation refers other RESPV3 type support, I'm really not sure whether it works. Plz review my code. Thanks

kvrocks.conf Outdated Show resolved Hide resolved
@git-hulk
Copy link
Member

@wsehjk You can search addReplyVerbatim inside Redis codebase to see where's using the verbatim string and also replace it in Kvrocks.

@apache apache deleted a comment from git-hulk Feb 14, 2024
@PragmaTwice
Copy link
Member

Repeated comment deleted.

@wsehjk wsehjk requested a review from PragmaTwice February 17, 2024 01:53
@wsehjk
Copy link
Contributor Author

wsehjk commented Feb 17, 2024

@wsehjk
Copy link
Contributor Author

wsehjk commented Feb 17, 2024

Code updated.

@PragmaTwice PragmaTwice requested a review from git-hulk February 17, 2024 13:20
@PragmaTwice
Copy link
Member

PragmaTwice commented Feb 17, 2024

@wsehjk You can search addReplyVerbatim inside Redis codebase to see where's using the verbatim string and also replace it in Kvrocks.

@wsehjk As mentioned by @git-hulk , besides adding the reply function, we need to replace it in existing command impls.

@wsehjk
Copy link
Contributor Author

wsehjk commented Feb 17, 2024

@wsehjk You can search addReplyVerbatim inside Redis codebase to see where's using the verbatim string and also replace it in Kvrocks.

@wsehjk As mentioned by @git-hulk , besides adding the reply function, we need to replace it in existing command impls.

Well, I figure out what you mean, wan't ware of this additional work before. I will work on this. But can we do this in a seperate issue and merge this pr #2102?

@git-hulk
Copy link
Member

@wsehjk After taking a look at addReplyVerbatim, only one place that needs to be changed in Kvrocks: use verbatim string while replying to the cluster command: https://github.com/redis/redis/blob/c854873746e9515f322d805f863e839ebfcefd62/src/cluster.c#L853

And for the Lua part, I will do it in the next PR.

@wsehjk
Copy link
Contributor Author

wsehjk commented Feb 17, 2024

@wsehjk After taking a look at addReplyVerbatim, only one place that needs to be changed in Kvrocks: use verbatim string while replying to the cluster command: https://github.com/redis/redis/blob/c854873746e9515f322d805f863e839ebfcefd62/src/cluster.c#L853

And for the Lua part, I will do it in the next PR.

what about the info cmd and client cmd https://github.com/redis/redis/blob/c854873746e9515f322d805f863e839ebfcefd62/src/server.c#L6082
https://github.com/redis/redis/blob/c854873746e9515f322d805f863e839ebfcefd62/src/networking.c#L3086

@git-hulk
Copy link
Member

@wsehjk After taking a look at addReplyVerbatim, only one place that needs to be changed in Kvrocks: use verbatim string while replying to the cluster command: https://github.com/redis/redis/blob/c854873746e9515f322d805f863e839ebfcefd62/src/cluster.c#L853
And for the Lua part, I will do it in the next PR.

what about the info cmd and client cmd https://github.com/redis/redis/blob/c854873746e9515f322d805f863e839ebfcefd62/src/server.c#L6082 https://github.com/redis/redis/blob/c854873746e9515f322d805f863e839ebfcefd62/src/networking.c#L3086

Yes, can fix them as well.

@wsehjk
Copy link
Contributor Author

wsehjk commented Feb 18, 2024

As discussed before, I replace redis::BulkString with connection->VerbatimString()reply in info, client and clustercommand where in redis codebae addReplyVerbatim reply is used

Copy link

sonarcloud bot commented Feb 18, 2024

Quality Gate Failed Quality Gate failed

Failed conditions
51.5% Coverage on New Code (required ≥ 60%)

See analysis details on SonarCloud

@git-hulk git-hulk requested a review from PragmaTwice February 18, 2024 07:21
@git-hulk git-hulk changed the title RESP3 verbatim string support Add support of RESP3 verbatim string Feb 18, 2024
@git-hulk git-hulk merged commit 7fe2bc2 into apache:unstable Feb 18, 2024
29 of 30 checks passed
@wsehjk
Copy link
Contributor Author

wsehjk commented Feb 18, 2024

Thanks

@git-hulk
Copy link
Member

@wsehjk Thanks for your contributions!

@wsehjk wsehjk deleted the dev branch February 20, 2024 07:13
JoverZhang pushed a commit to JoverZhang/kvrocks that referenced this pull request Feb 24, 2024
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.

Add RESP3 verbatim string type
3 participants