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

Support sharded pubsub commands #2498

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

vandyvilla
Copy link

@vandyvilla vandyvilla commented Jun 29, 2023

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
Copy link
Collaborator

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
Copy link
Collaborator

mgravell commented Aug 16, 2023

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

var channel = items[1].AsRedisChannel(ChannelPrefix, RedisChannel.PatternMode.Literal);
Trace("MESSAGE: " + channel);
RedisChannel channel;
if (items[0].IsEqual(message)) {
Copy link
Collaborator

@mgravell mgravell Aug 16, 2023

Choose a reason for hiding this comment

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

feels like we're testing this a lot; can we hoist this to a bool somewhere? maybe:

if (items.Length >= 3 && IsSimpleMessage(items[0], out bool isSharded))

then just use isSharded in all the places? for example channel = items[1].AsRedisChannel(ChannelPrefix, RedisChannel.PatternMode.Literal, isSharded);

or alternatively, using the PatternMode approach, maybe we can out var patternMode ?

@@ -485,6 +489,7 @@ private bool UnregisterSubscription(in RedisChannel channel, Action<RedisChannel
return false;
}

// TODO: We need a new api to support SUNSUBSCRIBE all. Calling this now would unsubscribe both sharded and unsharded channels.
Copy link
Collaborator

Choose a reason for hiding this comment

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

do we actually need such an API?

Copy link
Author

Choose a reason for hiding this comment

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

currently UnsubscribeAll will unsubscribe both unsharded and sharded channels. Alternatively, we need to pass some flag to indicate whether we want to unsubscribe all sharded channels to match the SUNSUBSCRIBE semantics.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I understand that. I just don't know that it is obvious whether that represents a problem. What genuinely likely scenario are we thinking of where this nuance is needed?

@@ -1325,7 +1325,7 @@ protected override bool SetResultCore(PhysicalConnection connection, Message mes
{
case ResultType.MultiBulk:
var final = result.ToArray(
(in RawResult item, in ChannelState state) => item.AsRedisChannel(state.Prefix, state.Mode),
(in RawResult item, in ChannelState state) => item.AsRedisChannel(state.Prefix, state.Mode, isSharded: false),
Copy link
Collaborator

Choose a reason for hiding this comment

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

do we need to support PUBSUB SHARDCHANNELS (see SubscriptionChannels[Async]) ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

ditto SHARDNUMSUB - see SubscriptionPatternCount / SubscriptionSubscriberCount

Copy link
Collaborator

Choose a reason for hiding this comment

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

ditto ssub in CLIENT - see ClientInfo.TryParse

Copy link
Author

Choose a reason for hiding this comment

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

yes, plan to add it in a separate PR.

Copy link
Collaborator

Choose a reason for hiding this comment

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

ssub handled in my extensions to this PR

@vandyvilla
Copy link
Author

vandyvilla commented Aug 23, 2023

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
Copy link

@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
Copy link
Collaborator

mgravell commented Mar 1, 2024

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
Copy link
Collaborator

mgravell commented Mar 4, 2024

I've taken the liberty of pushing the merge fixes; I've also proposed the single-field refactor here: vandyvilla#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
Copy link
Collaborator

mgravell commented Mar 4, 2024

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

image

@mgravell
Copy link
Collaborator

mgravell commented Mar 4, 2024

found and fixed; that was a genuine bug in RedisDatabase

image

@mgravell
Copy link
Collaborator

mgravell commented Mar 4, 2024

right, I think that's everything - @vandyvilla if you want to merge vandyvilla#1, then I should be able to pull in the combined effort (preserving your credit, etc)

@mgravell
Copy link
Collaborator

mgravell commented Mar 4, 2024

important TODOs before final merge:

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

@caoyang1024
Copy link

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

@nevermore-LQ
Copy link

@mgravell is this mr still active?

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.

5 participants