Add support for SORT_RO #2111
Add support for SORT_RO #2111
Conversation
| { | ||
| var command = destination.IsNull && multiplexer.GetServer(multiplexer.GetEndPoints()[0]).Version >= RedisFeatures.v7_0_0_rc1 ? RedisCommand.SORT_RO : RedisCommand.SORT; |
There was a problem hiding this comment.
Here we'd want to add a new internal member to RedisFeatures, e.g. internal bool SortedSetPop => Version >= v7_0_0_rc1; and use var features = GetFeatures(key, flags, out ServerEndPoint? server); like other places - note this means we need to pass server outward and use it upstream (to ensure we went to the server we said had the feature).
These types of switches are the only things we want to be added to RedisFeatures overall but let's keep internal for now.
There was a problem hiding this comment.
Thanks, @NickCraver, that's much cleaner, I'll clean up my other PR that does that hocky junk as well.
Co-authored-by: Nick Craver <nrcraver@gmail.com>
We can skip feature detection when it won't matter here :)
This can be a cleaner switch expression these days, yay!
|
Btw @NickCraver, in the |
|
@slorello89 I was wondering about that - are we saying SORT in general doesn't work against a replica ever? |
|
Aside, but it'd be really nice if the docs pages had an indicator of what is/isn't allowed on a read-only server somewhere... |
|
@NickCraver in theory the WRITE tag in the commands.json file is supposed to tell you that, you can see it under the ACLs in Redis.io |
|
Gotcha, I couldn't tell if that's "just with STORE" though - need to poke at this and test. If it can operate on a replica without STORE, then we shouldn't move/enforce that as it could break an existing use. |
|
@banker @itamarhaber - I like @NickCraver 's suggestion above - maybe we could add a table to redis.io to make it a bit clearer for our library developers what commands are meant to be write commands? |
These were hitting the primary so we weren't really exercising what we wanted to here for sure - made some tweaks to make damn sure we're testing what we think we are :)
…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 <nrcraver@gmail.com>
Introducing
SORT_ROas part of #2055@NickCraver - I modified the path for message creation for the Sort command to point to using
SORT_ROwhen possible, this again had to do a version-check against the multiplexer, which I'm not certain is the correct way - thoughts?Also, SORT is considered a write command and will be rejected out of hand by a replica (so I'm moving that as well)