Repository navigation
INCREX and ARGREP are routed by key on a cluster - #3352
Merged
Merged
Conversation
IncrexMessageBase (#3065) and ArrayGrepMessage (#3076) derived from Message directly, held a key, and never overrode GetHashSlot, so they reported NoSlot. On a cluster that sends the command to an arbitrary node and leans on -MOVED: an extra round trip for most calls, and a RedisServerException under CommandFlags.NoRedirect. Both now derive from Message.CommandKeyBase, as their single-key siblings do, which supplies the slot (and the key in CommandAndKey). Affects the StringIncrement overloads that take an Expiration (INCREX) and ArrayGrep. No public API change.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
INCREXandARGREPwere not routed by their key on a cluster. This fixes both.The problem
IncrexMessageBase(#3065) andArrayGrepMessage(#3076) derive fromMessagedirectly, hold aRedisKey, and never overrideGetHashSlot, so they reportNoSlot. On a cluster the command goes to an arbitrary node and depends on-MOVED:CommandFlags.NoRedirectthe call fails:Key has MOVED to Endpoint 127.0.0.1:7001 and hashslot 9615 but CommandFlags.NoRedirect was specified - redirect not followed for INCREX.Measured before the fix against the 6-node test cluster (3 primaries), 60 keys per row; the controls are single-key commands whose messages do carry a slot:
INCRBY(control)NoRedirectINCREXNoRedirectINCREXARGET(control)NoRedirectARGREPNoRedirectARGREPAffected surface: the
StringIncrement/StringIncrementAsyncoverloads that take anExpiration, andArrayGrep/ArrayGrepAsync.The fix
Both classes now derive from
Message.CommandKeyBase, as their single-key siblings do. That supplies the slot, and also puts the key inCommandAndKeyfor diagnostics. No public API change.Tests
KeyedMessageSlotTests:INCREXlong and double messages and theARGREPmessage report the key's slot;require: v8_8_0): 32 keys each underNoRedirect, so a call that misses the slot owner throws.All 5 fail with the fix reverted and pass with it, on net10.0 and net8.0. The existing
Increx*andArray*classes still pass (89 of 89 with the new tests, net10.0). The full suite and net481 were not run on this branch.Not in scope
An audit of the other message kinds found no other keyed message without a slot. It did find multi-key commands where only one key is hashed (
LMOVE,LMOVEM,ZRANGESTORE,GEOSEARCHSTORE,COPY ... DB,LCS ... IDX,LMPOP,ZMPOP,BITOP): same-slot calls route correctly, and cross-slot calls get the server'sCROSSSLOTrather than the library's multi-slot exception. Those are deliberately left alone here.