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

Cluster: Preserve message and suberrors when raising CommandErrorCollection #1238

Merged

Conversation

KJTsanaktsidis
Copy link
Contributor

The constructor signature for CommandErrorCollection is (errors, message). That means if you call "raise CommandErrorCollection, 'foobar'", that actually winds up setting the errors field to 'foobar', NOT the message. This leads to incredibly undescriptive "Command errors were replied on any node" errors getting emitted if there are errors using pipelining.

Fix this by:

  1. explicitly constructing the error, with the arguments in the right order
  2. mapping the sub-errors into redis-rb errors too - presumably there's an :errors attr_reader because somebody will find that useful.

…ection

The constructor signature for CommandErrorCollection is (errors,
message). That means if you call "raise CommandErrorCollection,
'foobar'", that actually winds up setting the _errors_ field to
'foobar', _NOT_ the message. This leads to incredibly undescriptive
"Command errors were replied on any node" errors getting emitted if
there are errors using pipelining.

Fix this by:
1) explicitly constructing the error, with the arguments in the right
order
2) mapping the sub-errors into redis-rb errors too - presumably there's
an :errors attr_reader because somebody will find that useful.
@KJTsanaktsidis KJTsanaktsidis force-pushed the ktsanaktsidis/make_error_collection_useful branch from f639f1a to 07e7849 Compare November 17, 2023 04:10
@byroot
Copy link
Collaborator

byroot commented Nov 17, 2023

I'll wait for @supercaracal 's opinion.

Copy link
Contributor

@supercaracal supercaracal left a comment

Choose a reason for hiding this comment

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

Sorry for my late reply. I agree with this fix.

@byroot
Copy link
Collaborator

byroot commented Nov 17, 2023

Not late at all. Thank you for maintaining clustering.

@byroot byroot merged commit a592a63 into redis:master Nov 17, 2023
16 of 17 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.

None yet

3 participants