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

adding some smarts to GetFeatures #2191

Merged
merged 4 commits into from
Aug 2, 2022
Merged

Conversation

slorello89
Copy link
Collaborator

@slorello89 slorello89 commented Jul 18, 2022

@NickCraver - This will fix #2016

The issue is that when GetFeatures does the server selection, it uses PING as the command it's asking about, so when the multiplexer responds with a server, it responds with any server. Of course, then when it goes to execute the command, it's been explicitly handed a server, so it honors that choice but checks it to make sure it's a valid server to send the command to, so if you're executing a write command and GetFeatuers just so happened to output a read-only replica, when the multiplexer does the 'is this a valid server?' check, it determines it is not and propagates the error. that's what causes the blowup. By passing in the RedisCommand to GetFeatures, we allow it to make an informed choice of server for that Redis Command.

The other option is to simply not pass the server on down the line when we we've done a feature check, and allow the muxer to make it's own decision, this would cause an extra run of Select which the current pattern e.g. see sort tries to avoid

One weird case, with SORT/SORT_RO, if the RO command is what we prefer to run (because we don't have a destination key to output it to), and SORT_RO is not available (because they aren't on Redis 7+), we cannot trust the output of GetFeatures, so I just set the selected server back to null and let the muxer figure it out down the line.

The rub with this PR, though is that I'm not 100% sure about how to go about testing this - do you have any thoughts?

@NickCraver NickCraver marked this pull request as ready for review August 2, 2022 02:11
@NickCraver
Copy link
Collaborator

@slorello89 had to find time to wrap my head around it - I don't think there's a good existing way to ensure this, will keep thinking on it, but I see the issue/fix and don't think we should delay getting better in place while we figure out regression testing on it :)

@slorello89
Copy link
Collaborator Author

Sounds good @NickCraver

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.

RedisCommandException for "Command cannot be issued to a replica" is difficult to handle
3 participants