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

Updating IsPrimaryOnly command map for new commands #2101

Merged
merged 9 commits into from
Apr 19, 2022

Conversation

slorello89
Copy link
Collaborator

@NickCraver, working on BITFIELD/BITFIELD_RO and I realized that several of the new commands we've been adding are write-commands and therefore won't work on replicas, opening this to fix the command-map in Message.cs (pretty sure this is the right place but LMK if I'm offbase here) will need to push an update to #2095 #2094 to reflect this as well.

@NickCraver
Copy link
Collaborator

@slorello89 agreed, but seems like we need a decent way of testing this with .DemandReplica assuring it throws. How to best make sure we test new methods either way though, hmmm...

@slorello89
Copy link
Collaborator Author

@NickCraver

What about something like this for each write-command in question?

I suppose it doesn't demonstrate that the command will automatically target the primary, but that's probably out of scope for what an individual command is expected to demonstrate.

@slorello89
Copy link
Collaborator Author

@NickCraver, if you wanted to test this for all commands, implement a general solution, you'd need to add a test that has a default implementation for each command, attempts to demand replica on each, and breaks or doesn't based on it's validity.

@NickCraver
Copy link
Collaborator

@slorello89 Oh sorry I didn't mean add a new test per (I think one global test works here) - it's more of ensuring a command is added to the primary check itself - like it has to be there or not. I'll try and play tonight on some options like you have to declare true/false or we throw and the test wouldn't pass, etc.

We could for instance have 1 method with all commands that has a giant switch of true/false returns and defaults to throw - if you don't add it to the test and the test itself doesn't match is IsPrimaryOnly from Message, it fails - this would ensure we account for it. Or...some other approach?

@slorello89
Copy link
Collaborator Author

You'd probably want to iterate over all the RedisCommand Enums, have some default implementation, try to demand replica, and check to make sure it fails when expected, or otherwise passes?

@NickCraver
Copy link
Collaborator

NickCraver commented Apr 18, 2022

@slorello89 I think this can be simpler by baking it in at the root level. We can change IsPrimaryOnly to have all commands that should return false as a new chunk, and make the default throw. Then you can't add a command without accounting for it, pretty much nothing would work.

...and yeah, we should have a test that loops over all redis commands making a message, just to be sure anything calls it to trip this going off.

@NickCraver
Copy link
Collaborator

Oh I see IsPrimaryOnly is static, we can just loop and call it :)

@slorello89
Copy link
Collaborator Author

@NickCraver ahah, I gotcha, yeah that makes sense

@slorello89
Copy link
Collaborator Author

Ok, I've worked out how to do this, I'll work on that update & test tonight/tomorrow. Shouldn't be terribly difficult.

@NickCraver
Copy link
Collaborator

@slorello89 Have a spare hour an ideas - poking at this :)

This makes maintenance easier since the place you add a command is right above where it must be a member rather than hidden elsewhere. Tweaking an existing check ensures it works.
@NickCraver
Copy link
Collaborator

@slorello89 Alrighty, pushed up for eyes!

@slorello89
Copy link
Collaborator Author

Looks about right. Let me take a pass though. It looks like some commands weren't added to the demand-primary loop when they were added. Some commands in there that do writes that shouldn't be passed onto a replica

Copy link
Collaborator Author

@slorello89 slorello89 left a comment

Choose a reason for hiding this comment

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

Yeah definitely a few in here that are write commands, likely weren't explicitly added to this list when they were added to the library - these changes ought to take care of that

src/StackExchange.Redis/Enums/RedisCommand.cs Show resolved Hide resolved
src/StackExchange.Redis/Enums/RedisCommand.cs Outdated Show resolved Hide resolved
src/StackExchange.Redis/Enums/RedisCommand.cs Outdated Show resolved Hide resolved
src/StackExchange.Redis/Enums/RedisCommand.cs Outdated Show resolved Hide resolved
src/StackExchange.Redis/Enums/RedisCommand.cs Outdated Show resolved Hide resolved
src/StackExchange.Redis/Enums/RedisCommand.cs Outdated Show resolved Hide resolved
src/StackExchange.Redis/Enums/RedisCommand.cs Show resolved Hide resolved
@NickCraver
Copy link
Collaborator

@slorello89 oh yeah I noticed some stream for sure that looked like it - agree the list needs tweaking to be correct in the catch-up here.

Goal is making contributing easier, so more comments here!
@NickCraver
Copy link
Collaborator

NickCraver commented Apr 19, 2022

Hmmm, @slorello89 this sucks :) Not your code, but the reality of "fixing" this means we give a crap message. Compare, today when this gets through:

RedisServerException : READONLY You can't write against a read only replica.

If we actively block it (this change):

RedisConnectionException: No connection is active/available to service this operation: XADD auto_id_stream_1650328241728, mc: 1/1/0, mgr: 10 of 10 available, clientName: StreamOpsFailOnReplica, IOCP: (Busy=0,Free=1000,Min=16,Max=1000), WORKER: (Busy=2,Free=2045,Min=16,Max=2047), v: 2.6.26.52977

I think we need to change the messaging when we have a connection, but not a writable one - that's not intuitive IMO. So I'm for fixing the issue here, but we need to improve the exception along with it. Maybe as simple as "No writable connection is..."? Thoughts/suggestions?

@NickCraver
Copy link
Collaborator

okay this is an oddity in the flow for a non-specific server otherwise we throw a better message here:

if (message.IsPrimaryOnly() && server.IsReplica)
{
    throw ExceptionFactory.PrimaryOnly(RawConfig.IncludeDetailInExceptions, message.Command, message, server);
}

...will noodle on how we re-work that to cover the non-specified case.

@slorello89
Copy link
Collaborator Author

I suppose the other wrinkle we need to consider, what about writeable replicas? Would this be BC for the stream commands on writeable replicas? I'd think so right?

@NickCraver
Copy link
Collaborator

I suppose the other wrinkle we need to consider, what about writeable replicas? Would this be BC for the stream commands on writeable replicas? I'd think so right?

Sooooooo I'm okay punting on that, because it doesn't work now. We've inadvertently blocked that in the IsPrimaryOnly() regardless of settings due to it being command-based for years. It's an issue in #354. I don't think anything is being made worse here - we're just making IsPrimaryOnly() complete here. The issue needs a more global solution but I don't think it's made more difficult by anything in here.

@NickCraver
Copy link
Collaborator

@slorello89 I pushed some goodies here on the message and started a Streams test - can go through the remaining commands and test each and add to that method before moving. Thoughts on hopefully-improved error messaging for users here?

@NickCraver
Copy link
Collaborator

@slorello89 Alrighty pushed changes and verification up (was testing each before adding to the list) - ready for another pass!

Can apply the same love to many other classes to be clear but will make another PR for it.
Copy link
Collaborator

@NickCraver NickCraver left a comment

Choose a reason for hiding this comment

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

Looks good, but I've heavily tainted my review - @mgravell can you lay eyes please?

Copy link
Collaborator

@mgravell mgravell left a comment

Choose a reason for hiding this comment

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

seems legit

@NickCraver NickCraver merged commit ecea7f4 into main Apr 19, 2022
@NickCraver NickCraver deleted the slorello89/IsPrimaryCommandMap branch April 19, 2022 15:06
NickCraver added a commit that referenced this pull request Jul 18, 2022
…primary-only out of primary-only (#2183)

Fixes #2182 

@NickCraver for background in #2101, we changed some of the logic around primary-only to require all commands to be explicitly stated as primary-only or not. During this effort, I went through the command list and checked to see which were considered write commands (which would be rejected by a replica) so that there would be consistency in behavior when stumbling on commands that would ordinarily be rejected by a replica. This made a slightly more restrictive list of commands. 

Enter #2182, where this inconsistency had evidently become load-bearing. The user has evidently designated their replicas as writeable (I think I was subconsciously aware of this capability but have never actually seen anyone use it). As it turns out By resolving this inconsistency in #2101 and the follow-on issue when I introduced `SORT_RO` in #2111 I apparently introduced a break.

This PR reverts all the pre-2.6.45 commands' primary vs replica disposition back to their previous state which will remove the break. Any command that was introduced in 2.6.45 is correctly dispositioned.

Co-authored-by: Nick Craver <[email protected]>
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