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

match appropriate error message in Client.connect rescue CommandError #1025

Merged
merged 1 commit into from Sep 23, 2021

Conversation

verajohne
Copy link
Contributor

Following #990 all CommandErrors are being rescued here, including max connections resulting in Redis::CommandError: ERR max number of clients reached being rescued here and eventually rasied as Redis::ConnectionError: Connection lost (ECONNRESET).

It is my understanding that #990 was a fix to #989 and specifically Redis::CommandError: ERR wrong number of arguments for 'auth' command. We should instead match on this error message, and otherwise raise.

@verajohne verajohne changed the title match approvopiate error message in Client.connect rescue CommandError match appropriate error message in Client.connect rescue CommandError Sep 6, 2021
@byroot
Copy link
Collaborator

byroot commented Sep 6, 2021

Can you please squash your commits?

As for the rubocop failure, I'll change the config, that rule is silly.

@verajohne verajohne force-pushed the rescue_command_error_match_message branch from ef27116 to 17ba094 Compare September 7, 2021 14:24
@verajohne
Copy link
Contributor Author

@byroot I squashed the commits, could we merge this?

@byroot
Copy link
Collaborator

byroot commented Sep 23, 2021

Sure I'll merge on green. Sorry I didn't see your squash until now, I'm not notifed on push, that's just way too many notifications, I should have told you to ping me when you were done.

@verajohne
Copy link
Contributor Author

Sure I'll merge on green. Sorry I didn't see your squash until now, I'm not notifed on push, that's just way too many notifications, I should have told you to ping me when you were done.

Totally get that :) Thank you!

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

2 participants