cachedb_redis: add Redis cluster hash tag support to redisHash#3815
cachedb_redis: add Redis cluster hash tag support to redisHash#3815NormB wants to merge 1 commit intoOpenSIPS:masterfrom
Conversation
Fix redisHash() to extract hash tags per the Redis cluster spec and use the spec-mandated CRC16(key) mod 16384 bitmask.
|
Thanks for flagging this @NormB You're right — the hash slot calculation fix overlaps with #3815. Looking at your PR, it's actually more thorough on that front since it also adds hash tag {…} extraction, which mine doesn't cover. The main contribution of this PR is the ASK redirect handling — when a Redis cluster is mid-resharding, source nodes return ASK instead of MOVED, and this requires sending an ASKING command to the target node before retrying. This completes the cluster redirect support started in #3639. That part has no overlap with #3815. Happy to rebase this PR to remove the hash slot fix and keep only the ASK redirect handling, so it can sit cleanly on top of #3815. Would that work, or would you prefer I fold both into one PR? |
|
Thanks @dondetir — rebasing to keep only the ASK redirect handling sounds like the right approach. Your I have two follow-up PRs that build on top of both #3815 and your ASK work:
|
Fix redisHash() to extract hash tags per the Redis cluster spec and use the spec-mandated CRC16(key) mod 16384 bitmask.
Summary
Fix redisHash() in cachedb_redis to support Redis cluster hash tags ({...}) and use the spec-mandated slot modulus.
Details
The redisHash() function in modules/cachedb_redis/cachedb_redis_utils.c has two correctness issues when compared to the https://redis.io/docs/latest/operate/oss_and_stack/reference/cluster-spec/:
Missing hash tag {...} extraction — Redis cluster keys containing {substring} should hash only the substring inside the first {...} pair, enabling co-location of related keys on the same slot. The current code always hashes the full key. This means keys like {user:1000}.session and {user:1000}.profile, which are designed to land on the same slot, get scattered across different slots.
Uses & con->slots_assigned instead of & 16383 — slots_assigned is set to the max end_slot found across nodes (which happens to be 16383 in a fully-allocated cluster), but this is fragile and semantically wrong. The Redis spec mandates CRC16(key) mod 16384 (i.e., & 16383).
Standalone test results confirming the bug and fix:
CRC16 check: crc16("123456789") = 0x31C3 (expected 0x31C3) PASS
KEY OLD NEW STATUS
foo 12182 12182 PASS (regression safe)
bar 5061 5061 PASS (regression safe)
hello 866 866 PASS (regression safe)
{user:1000}.session 10291 1649 PASS (old=BROKEN, new=FIXED)
{user:1000}.profile 11999 1649 INFO (slot changed)
{user:1000}.followers 4151 1649 PASS (old=BROKEN, new=FIXED)
foo{} 5542 5542 PASS (regression safe)
{}key 14961 14961 PASS (regression safe)
foo{{bar}} 4285 4015 INFO (slot changed)
foo{bar}{zap} 4770 5061 INFO (slot changed)
Results: 10 passed, 0 failed
Solution
The redisHash() function now implements hash tag extraction matching the https://redis.io/docs/latest/operate/oss_and_stack/reference/cluster-spec/ from the Redis cluster spec:
Additionally adds a NULL/empty key guard with LM_ERR and debug logging (LM_DBG) when a hash tag is detected. The function signature is unchanged — no caller modifications needed.
Compatibility
Plain keys (no {...}) produce identical slot values — no behavioral change for existing deployments not using hash tags.
Keys containing {...} hash tags will now route to different (correct) slots. Any deployment relying on the old (broken) slot assignment for hash-tagged keys would see keys move to new slots on restart. This is the correct behavior per the Redis spec and matches what redis-cli CLUSTER KEYSLOT returns.
The slots_assigned field in redis_con is retained — it is still populated during cluster discovery but no longer used for the bitmask. Removing it would be a separate cleanup.
Closing issues