Hey there! Thanks for getting this in :)

I'll try and get eyes on this week, but wanted to scan quick and add some top level thoughts:

1. I think we can remove the config option here - we're detecting if it's Azure Redis already so we can use that to determine whether to hook up to events or not.
2. I think we can simplify a lot of the objects here, e.g. MaintenanceNotificationListener being simplified to a single method like AzureMaintenanceEvent.AddListener(ConnectionMultiplexer muxer, string cause) (or whatever overload makes sense).
   • Also applies to ReconfigureAfterMaintenance - we can decide whether parsing the event is needed or not
3. I think we can simplify the looping on the pipe-delimited message to be span-based for an easier-to-understand iterator.
4. In testing, doing inline Assert.True() instead of if...return false statements in ValidateAzureMaintenanceEvent will make for easier testing - we'll see exactly which check failed by line number in test output that way. If it's false, it'll blow up and not continue the method so that's alright, IMO.

@NickCraver Thanks so much for the quick reply! I have a couple questions/comments:

1. Philo mentioned that we'll automatically subscribe to these events for anyone who is using the Azure.StackExchange.Redis extension package. However since subscribing to the pubsub creates another connection, we want this to be opt-in only in order not to burden folks who might already be short on connections. Thoughts?

2. Could you point me to a place in the code or a doc that illustrates what span-based iteration looks like? Not sure I'm familiar with that type/concept!

Working on addressing the other feedback now :)

Oooh gotcha - alrighty maybe a mismatch on comms here between me and @philon-msft - let me clarify and see if we agree.

On 1: I'm of the opinion we can go ahead and opt everyone into this in the current package if we think we're connecting to Azure. We could make the method for doing so explicitly public so that any non-detectable/custom endpoints are easily opted in for anyone in that situation. I don't think we have to wait for a consumer package move. It won't create another connection than it does today because we already light up the subscriber connection unconditionally on the ServerEndPoint - ultimately you're looking here - called when we first connect.

On 2: I took a stab at illustrating this and...meh, it's kind of a wash. Here's what I ended up with:

/// <summary>
/// Azure node maintenance event details.
/// </summary>
public class AzureMaintenanceEvent
{
	public AzureMaintenanceEvent(ReadOnlySpan<char> message)
	{
		try
		{
			while (message.Length > 0)
			{
				if (message[0] == '|')
				{
					message = message.Slice(1);
					continue;
				}

				// Grab the next pair
				var key = message.Slice(0, message.IndexOf('|'));
				message = message.Slice(key.Length + 1);

				var valueEnd = message.IndexOf('|');
				var value = valueEnd > -1 ? message.Slice(0, valueEnd) : message;
				message = message.Slice(value.Length);

				if (key.Length > 0 && value.Length > 0)
				{
					switch (key)
					{
						case var _ when key.SequenceEqual(nameof(NotificationType)):
							NotificationType = value.ToString();
							break;
						case var _ when key.SequenceEqual(nameof(StartTimeUtc)) && DateTime.TryParse(value, out DateTime startTime):
							StartTimeUtc = DateTime.SpecifyKind(startTime, DateTimeKind.Utc);
							break;
						case var _ when key.SequenceEqual(nameof(IsReplica)) && bool.TryParse(value, out var isReplica):
							IsReplica = isReplica;
							break;
						case var _ when key.SequenceEqual(nameof(IPAddress)) && IPAddress.TryParse(value, out var ipAddress):
							IpAddress = ipAddress;
							break;
						case var _ when key.SequenceEqual(nameof(SSLPort)) && Int32.TryParse(value, out var port):
							SSLPort = port;
							break;
						case var _ when key.SequenceEqual(nameof(NonSSLPort)) && Int32.TryParse(value, out var nonsslport):
							NonSSLPort = nonsslport;
							break;
						default:
							break;
					}
				}
			}
		}
		catch { }
	}

	/// <summary>
	/// The type of event.
	/// </summary>
	public readonly string NotificationType;

	/// <summary>
	/// The start time of the event.
	/// </summary>
	public readonly DateTime? StartTimeUtc;

	/// <summary>
	/// Whether the event is for a replica node.
	/// </summary>
	public readonly bool IsReplica;

	/// <summary>
	/// IPAddress of the node event is intended for
	/// </summary>
	public readonly IPAddress IpAddress;

	/// <summary>
	/// SSL port
	/// </summary>
	public readonly int SSLPort;

	/// <summary>
	/// Non-SSL Port
	/// </summary>
	public readonly int NonSSLPort;

	/// <summary>
	/// Returns a string representing the maintenance event with all of its properties.
	/// </summary>
	public override string ToString() =>
		$"{nameof(NotificationType)}|{NotificationType}|" +
			$"{nameof(StartTimeUtc)}|{StartTimeUtc:s}|" +
			$"{nameof(IsReplica)}|{IsReplica}|" +
			$"{nameof(IpAddress)}|{IpAddress}|" +
			$"{nameof(SSLPort)}|{SSLPort}|" +
			$"{nameof(NonSSLPort)}|{NonSSLPort}";
}

However, after doing that I don't really know that there's any reason to even parse it for our use case, so maybe we just record the string itself (for logs/events/whatever) and move along? If the only practical use of the event parsing is to have specifics we're turning back into a string for debugging...maybe we don't do any work on it at all. If we can log it as-is and that's equally readable as any .ToString() implementation: how about we simplify? We should be aiming to trigger the same reconfigure either way.

What are y'all thinking on these fronts?

@NickCraver thanks for explaining!

I removed the configuration option since we're not sparing any connections by having it around, as you pointed out. I also cleaned up the tests and condensed the classes into an AzureMaintenanceEvent.AddListenerAsync() method as suggested, and it's so much cleaner now! 😃

The only thing I had some concerns about is the parsing -- I'm not sure if I understood your response correctly (please do correct me if I'm wrong) -- I think we do want to parse the message into a class, since the user might want to do some additional logic based on which event comes back and when the start time is. As for whether we keep the pipe-based iteration or not, I'm happy to switch to using the span method if it's more intuitive!

@NickCraver thanks for the excellent review! I've replied to most of the comments, and updated this PR's description with more context (specifically re: parsing and exposing events). Let me know if it makes sense, and hopefully we can sync up if there's anything else we need to align on! :)

oh, can we update releasenotes.md too please, in the unreleased section? thanks

The DateTime parse is of particular concern to me; I wonder if the format should be explicit/"exact" here (i.e. DateTime.TryParseExact) - or is it sufficient to use the 'u' pattern (which is not the default) on Utf8Parser? Does 'u' act the same as "u"? (open question; I don't know the answer)

Hey @amsoedal! I figured we'd see you on call today but missed out on syncing up - figured it was easier to talk through some tweaks to make this more extensible to other providers later and not lock ourselves in on the new public types with a few changes. I have the proposals in craver/1865-tweaks but definitely don't want to stomp on PR and credit here. I've granted you write access to this repo - could you please move your branch here and open a new PR (I think all current comments are good to lose). Then we can open a PR from craver/1865-tweaks as a suggested PR to that branch :) Curious on thoughts here, and told @philon-msft I'm happy to hop on a call tomorrow to go through and finish this off if sync helps :)