-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Subscribe to all primaries (for key-space-notifications) #2860
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,6 +1,8 @@ | ||
using System; | ||
using System.Collections.Concurrent; | ||
using System.Collections.Generic; | ||
using System.Diagnostics.CodeAnalysis; | ||
using System.Linq; | ||
using System.Net; | ||
using System.Threading; | ||
using System.Threading.Tasks; | ||
|
@@ -432,6 +434,86 @@ public Task<bool> SubscribeAsync(RedisChannel channel, Action<RedisChannel, Redi | |
return EnsureSubscribedToServerAsync(sub, channel, flags, false); | ||
} | ||
|
||
Task ISubscriber.SubscribeAllPrimariesAsync(RedisChannel channel, Action<RedisChannel, RedisValue> handler, CommandFlags flags) | ||
=> SubscribeAllPrimariesAsync(channel, handler, null, flags); | ||
|
||
public Task<bool> SubscribeAllPrimariesAsync(RedisChannel channel, Action<RedisChannel, RedisValue>? handler, ChannelMessageQueue? queue, CommandFlags flags) | ||
{ | ||
ThrowIfNull(channel); | ||
if (handler == null && queue == null) { return CompletedTask<bool>.Default(null); } | ||
|
||
var sub = multiplexer.GetOrAddSubscription(channel, flags); | ||
sub.Add(handler, queue); | ||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. there's almost certainly something complex in here around channel reconnects... I'll need to dig There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. oww, now i see.. |
||
{ | ||
if (sub.IsConnected) { return CompletedTask<bool>.Default(null); } | ||
|
||
// TODO: Cleanup old hangers here? | ||
sub.SetCurrentServer(null); // we're not appropriately connected, so blank it out for eligible reconnection | ||
var tasks = new List<Task<bool>>(); | ||
foreach (var server in multiplexer.GetServerSnapshot()) | ||
{ | ||
if (!server.IsReplica) | ||
{ | ||
var message = sub.GetMessage(channel, SubscriptionAction.Subscribe, flags, internalCall); | ||
tasks.Add(ExecuteAsync(message, sub.Processor, server)); | ||
} | ||
} | ||
|
||
if (tasks.Count == 0) | ||
{ | ||
return CompletedTask<bool>.Default(false); | ||
} | ||
|
||
// Create a new task that will collect all results and observe errors | ||
return Task.Run(async () => | ||
{ | ||
// Wait for all tasks to complete | ||
var results = await Task.WhenAll(tasks).ObserveErrors(); | ||
return results.All(result => result); | ||
}).ObserveErrors(); | ||
} | ||
|
||
Task ISubscriber.UnsubscribeAllPrimariesAsync(RedisChannel channel, Action<RedisChannel, RedisValue>? handler, CommandFlags flags) | ||
=> UnsubscribeAllPrimariesAsync(channel, handler, null, flags); | ||
|
||
public Task<bool> UnsubscribeAllPrimariesAsync(in RedisChannel channel, Action<RedisChannel, RedisValue>? handler, ChannelMessageQueue? queue, CommandFlags flags) | ||
{ | ||
ThrowIfNull(channel); | ||
// Unregister the subscription handler/queue, and if that returns true (last handler removed), also disconnect from the server | ||
return UnregisterSubscription(channel, handler, queue, out var sub) | ||
? UnsubscribeFromPrimariesAsync(sub, channel, flags, asyncState, false) | ||
: CompletedTask<bool>.Default(asyncState); | ||
} | ||
|
||
private Task<bool> UnsubscribeFromPrimariesAsync(Subscription sub, RedisChannel channel, CommandFlags flags, object? asyncState, bool internalCall) | ||
{ | ||
var tasks = new List<Task<bool>>(); | ||
foreach (var server in multiplexer.GetServerSnapshot()) | ||
{ | ||
if (!server.IsReplica) | ||
{ | ||
var message = sub.GetMessage(channel, SubscriptionAction.Unsubscribe, flags, internalCall); | ||
tasks.Add(multiplexer.ExecuteAsyncImpl(message, sub.Processor, asyncState, server)); | ||
} | ||
} | ||
|
||
if (tasks.Count == 0) | ||
{ | ||
return CompletedTask<bool>.Default(false); | ||
} | ||
|
||
// Create a new task that will collect all results and observe errors | ||
return Task.Run(async () => | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I suspect we can unwrap a little of the extra task layer here, but I'm not too concerned - this is low throughput |
||
{ | ||
// Wait for all tasks to complete | ||
var results = await Task.WhenAll(tasks).ObserveErrors(); | ||
return results.All(result => result); | ||
}).ObserveErrors(); | ||
} | ||
public Task<bool> EnsureSubscribedToServerAsync(Subscription sub, RedisChannel channel, CommandFlags flags, bool internalCall) | ||
{ | ||
if (sub.IsConnected) { return CompletedTask<bool>.Default(null); } | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if we should infer "all primaries" (vs secondaries) from
flags
, like we would with regular subscribe, so this becomesSubscribeAll
; I also wonder whether we should internalize this entirely, specifically against the keyspace events, because for regular keys:s
variants don't make sense to attach to the entire cluster, since they are routed using shard rulesThat would also have the advantage of not needing a separate API. I'm not certain on this - mostly thinking aloud here.