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

Improve RESP handling code in replication #2334

Merged
merged 5 commits into from
May 29, 2024

Conversation

PragmaTwice
Copy link
Member

See changes.

@PragmaTwice PragmaTwice requested a review from git-hulk May 27, 2024 13:40
git-hulk
git-hulk previously approved these changes May 27, 2024
@@ -43,5 +43,7 @@ inline constexpr const char *errValueIsNotFloat = "value is not a valid float";
inline constexpr const char *errNoMatchingScript = "NOSCRIPT No matching script. Please use EVAL";
inline constexpr const char *errUnknownOption = "unknown option";
inline constexpr const char *errUnknownSubcommandOrWrongArguments = "Unknown subcommand or wrong number of arguments";
inline constexpr const char *errWrongNumArguments = "ERR wrong number of arguments";
Copy link
Member

Choose a reason for hiding this comment

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

Should we remove ERR here? ( Seems some str has Err, some are not..)

Copy link
Member Author

Choose a reason for hiding this comment

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

I keep it for convenience, since it should be included in both use sites.

Copy link
Member

@git-hulk git-hulk May 27, 2024

Choose a reason for hiding this comment

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

We cannot remove the ERR prefix since it may introduce a compatibility issue.

Seems some str has Err, some are not..

It's an old implementation issue, Redis uses ERR/LOADING/NOPROTO to identify different kinds of errors, but we didn't take care of this in the original implementation. So it's really hard to fix them in a short time.

refer: https://redis.io/docs/latest/develop/reference/protocol-spec/

mapleFU
mapleFU previously approved these changes May 27, 2024
git-hulk
git-hulk previously approved these changes May 28, 2024
Copy link

@PragmaTwice PragmaTwice merged commit 5e99b27 into apache:unstable May 29, 2024
32 checks passed
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.

3 participants