Subscribe to all primaries (for key-space-notifications)#2860
Subscribe to all primaries (for key-space-notifications)#2860atakavci wants to merge 1 commit intoStackExchange:mainfrom
Conversation
| /// <seealso href="https://redis.io/commands/subscribe"/>, | ||
| /// <seealso href="https://redis.io/commands/psubscribe"/>. | ||
| /// </remarks> | ||
| Task SubscribeAllPrimariesAsync(RedisChannel channel, Action<RedisChannel, RedisValue> handler, CommandFlags flags = CommandFlags.None); |
There was a problem hiding this comment.
I wonder if we should infer "all primaries" (vs secondaries) from flags, like we would with regular subscribe, so this becomes SubscribeAll; I also wonder whether we should internalize this entirely, specifically against the keyspace events, because for regular keys:
- non-'s' variants will get broadcast duplicates that will be hard to untangle
svariants don't make sense to attach to the entire cluster, since they are routed using shard rules
That would also have the advantage of not needing a separate API. I'm not certain on this - mostly thinking aloud here.
There was a problem hiding this comment.
I'll like the idea to having just a single Subscribe call that will identify if the Channel is a keyspace event or a regular Pub/Sub channel, that way the user doesn't have to handle the possible duplication of "regular" events and also doesn't have to manage to Connect to every single Node, which also agree we could say via flags whether to connect to master or replicas, or all if the user still wants that.
| } | ||
|
|
||
| // Create a new task that will collect all results and observe errors | ||
| return Task.Run(async () => |
There was a problem hiding this comment.
I suspect we can unwrap a little of the extra task layer here, but I'm not too concerned - this is low throughput
| return EnsureSubscribedToPrimariesAsync(sub, channel, flags, false); | ||
| } | ||
|
|
||
| private Task<bool> EnsureSubscribedToPrimariesAsync(Subscription sub, RedisChannel channel, CommandFlags flags, bool internalCall) |
There was a problem hiding this comment.
there's almost certainly something complex in here around channel reconnects... I'll need to dig
There was a problem hiding this comment.
oww, now i see..
we are missing the cases with failovers and etc..
i ll try to explore the cases. thanks.
this makes total sense and my attempt is solely for the sake of |
3 participants
Closes/Fixes #2846
This essentially uses the same approach as subscription methods but runs on all primary nodes in the cluster, attempting to combine the results into a single output.
While some parts of it align with existing pub/sub functionality, making them reusable compromises readability. Therefore, I'm not sure it's preferable to make them reusable.
Another point is that I went back and forth on API naming, trying to decide whether to reference the keyspace or not. To provide more clarity on what is happening, I chose a name -
SubscribeAllPrimariesAsync- that refers to the primaries, and I’ve kept the keyspace-related explanations in the documentation.