Support sharded pubsub commands

[vandyvilla]

Support SPUBLISH and SSUBSCRIBE, SUNSUBSCRIBE commands.

We tested this PR internally and will add more tests to this PR.

Would like to gather some early feedback. Thanks!

cc @NickCraver @mgravell

[mgravell]

RedisChannel seems to have a new bool field, increasing the size; to simplify design and avoid additional storage, since SSUBSCRIBE does not support wildcards, I wonder if we should implement this as a new PatternMode value instead of a separate bool - perhaps with a public static RedisChannel Sharded(...) pair to match the Literal and Pattern methods. At the moment there is a private readonly bool _isPatternBased, but ultimately this is padded to 4 bytes, so would be identical to private readonly PatternMode _mode;. As an alternative, we could use bit-bashing to combine the two bools as bits, to avoid increasing the size - but honestly I think the PatternMode approach is simpler.

My main concern, however, is around shard rebalancing; I don't see anything that handles this currently; presumably we'd need to detect shard movement, and validate that subscriptions are still against the correct server - right now I don't see that happening; we may or may not also need to do this during resubscribe against an existing connection?

[mgravell]

~~Does this actually work without error? I'm a little surprised because I would have expected CheckReadOnlyOperations to fail, because I don't think you've modified IsPrimaryOnly?~~

My bad; found them

[vandyvilla]

good idea for leveraging the pattern mode to save one additional bool variable.

Thanks for bringing up the shard movement concern, which is critical. Based on my testing, the subscriber gets a SUNSUBSCRIBE message reply from Redis when the shard is moved, which triggers a re-subscription. The new subscription against the old server will get a MOVED message, which makes it to resubscribe to the right server. Does this make sense to you?

[dmitrig89]

@mgravell No pressure, but any idea when we might expect this feature to be merged? We're currently experiencing performance bottlenecks when using the pub\sub pattern on a Redis cluster. I'm hopeful that sharding the pub\sub will help with the issues we're having

Thanks

[mgravell]

Sorry, took my eyes off this one; let's get this done! if you're not still eager, I can probably take it over; but!

1: I really don't like these constructors:

+ StackExchange.Redis.RedisChannel.RedisChannel(string! value, bool isSharded) -> void
+ StackExchange.Redis.RedisChannel.RedisChannel(byte[]? value, bool isSharded) -> void

can we instead use (to mirror .Literal and .Pattern:

public static RedisChannel Sharded(string value) => ...
public static RedisChannel Sharded(byte[] value) => ...

? To me that feels more consistent.

2: I still think we should squash the 2 bool, but: let's not worry about those today

[mgravell]

I've taken the liberty of pushing the merge fixes; I've also proposed the single-field refactor here: https://github.com/vandyvilla/StackExchange.Redis/pull/1 - would love to get your input / thoughts here @vandyvilla , although I guess I can push it through either way; sorry this has been delayed

[mgravell]

working on adding some tests; very much needed (this could be from my changes, note):

[mgravell]

found and fixed; that was a genuine bug in RedisDatabase

[mgravell]

right, I think that's everything - @vandyvilla if you want to merge https://github.com/vandyvilla/StackExchange.Redis/pull/1, then I should be able to pull in the combined effort (preserving your credit, etc)

[mgravell]

important TODOs before final merge:

• [ ] investigate break/reconnect logic
• [ ] investigate cluster shard migration logic
• [ ] investigate unsubscribe-all logic

[caoyang1024]

hi @NickCraver Could you please review this merge request at your earliest convenience?

[nevermore-LQ]

@mgravell is this mr still active?