Implement ROLE command by zmj · Pull Request #1551 · StackExchange/StackExchange.Redis

Conversation

zmj added 3 commits

August 13, 2020 19:14

		/// <param name="server">The server to simulate failure on.</param>
		public static void SimulateConnectionFailure(this IServer server) => (server as RedisServer)?.SimulateConnectionFailure();

		public static string Role(this IServer server)

Choose a reason for hiding this comment

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

Removed the existing test helper and changed the usage to use the new code

		/// <summary>
		/// This replica's replication state.
		/// </summary>
		public string State { get; }

Choose a reason for hiding this comment

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

This could be an enum, but returning the raw value seemed better for allowing the caller to do something useful with an unrecognized value.

		public void ReplicaRole()
		{
		var connString = $"{TestConfig.Current.ReplicaServerAndPort},allowAdmin=true";
		using var muxer = ConnectionMultiplexer.Connect(connString);

Choose a reason for hiding this comment

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

Is it safe to assume there is always a replica in the test run?

A few tests failed. This one looks like a bug: StackExchange.Redis.RedisCommandException : This operation has been disabled in the command-map and cannot be used: ROLE. I guess I need to add ROLE to one (or more) of the lists in CommandMap? Which ones are appropriate?

The others looked like network errors that were probably unrelated to this change.

On the failing test: look at public static CommandMap Sentinel

…

On Sat, 15 Aug 2020, 21:00 zmj, ***@***.***> wrote: A few tests failed. This one looks like a bug: StackExchange.Redis.RedisCommandException : This operation has been disabled in the command-map and cannot be used: ROLE. I guess I need to add ROLE to one (or more) of the lists in CommandMap? Which ones are appropriate? The others looked like network errors that were probably unrelated to this change. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#1551 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAEHMCMKMN3LYMXXRKJ7NDSA3SNHANCNFSM4QANWCMQ> .

zmj added 2 commits

August 15, 2020 14:36

zmj commented
Sep 12, 2020
•

Loading

I merged in the main branch's test changes. MultiMaster.TestMultiNoTieBreak is still failing. Doesn't seem related to this PR's changes. Any suggestions?

Conversation

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zmj commented Sep 12, 2020 • Loading

2 participants

zmj commented
Sep 12, 2020
•

Loading