On Sun, 7 Jun 2020, 15:56 Nick Craver, ***@***.***> wrote: ***@***.**** requested changes on this pull request. Found a few things inline and tried to note each. A few what look like API breaks, one possible mis-commit on the enum (or I'm confused). Overall I think the obsolete message isn't awesome though since it requires some extra effort on porting we could shave off. If cool with it, I'll go through and make that message more specific - thinking: [Obsolete(nameof(prop) + " is deprecated, please use " + nameof(newprop))] Thoughts? ------------------------------ In src/StackExchange.Redis/CommandMap.cs <#1488 (comment)> : > @@ -49,8 +49,8 @@ internal CommandMap(CommandBytes[] map) RedisCommand.ECHO, RedisCommand.PING, RedisCommand.QUIT, RedisCommand.SELECT, RedisCommand.BGREWRITEAOF, RedisCommand.BGSAVE, RedisCommand.CLIENT, RedisCommand.CLUSTER, RedisCommand.CONFIG, RedisCommand.DBSIZE, - RedisCommand.DEBUG, RedisCommand.FLUSHALL, RedisCommand.FLUSHDB, RedisCommand.INFO, RedisCommand.LASTSAVE, RedisCommand.MONITOR, RedisCommand.SAVE, - RedisCommand.SHUTDOWN, RedisCommand.SLAVEOF, RedisCommand.SLOWLOG, RedisCommand.SYNC, RedisCommand.TIME + RedisCommand.DEBUG, RedisCommand.FLUSHALL, RedisCommand.FLUSHDB, RedisCommand.INFO, RedisCommand.LASTSAVE, RedisCommand.MONITOR, RedisCommand.REPLICAOF, + RedisCommand.SAVE, RedisCommand.SHUTDOWN, RedisCommand.SLAVEOF, RedisCommand.SLOWLOG, RedisCommand.SYNC, RedisCommand.TIME Not for PR, but let's put these all on their own line for easier edits later (ones and group only related ones like above) ------------------------------ In src/StackExchange.Redis/ConnectionMultiplexer.cs <#1488 (comment)> : > @@ -429,19 +430,19 @@ internal void MakeMaster(ServerEndPoint server, ReplicationChangeOptions options } } - // deslave... + // de-replicate ⬇️ Suggested change - // de-replicate + // stop replicating, promote to a standalone primary ------------------------------ In src/StackExchange.Redis/ConnectionMultiplexer.cs <#1488 (comment)> : > Broadcast(nodes); - if ((options & ReplicationChangeOptions.EnslaveSubordinates) != 0) + if ((options & ReplicationChangeOptions.ReplicateToSubordinates) != 0) This one seems odd (but works) to me, the action isn't *from the master* as the verbage hints it, it's an action *on the subordinates*. Trying to think of a better name while reading this... ------------------------------ In src/StackExchange.Redis/Enums/ClientFlags.cs <#1488 (comment)> : > @@ -59,5 +69,17 @@ public enum ClientFlags : long /// connection to be closed ASAP /// CloseASAP = 256, + /// + /// the client is a Pub/Sub subscriber + /// + PubSubSubscriber = 512, + /// + /// the client is in readonly mode against a cluster node + /// + ReadOnlyCluster = 512, + /// + /// the client is connected via a Unix domain socket + /// + UnixDomainSocket = 512, Are these intended additions in this PR? Doesn't seem related...and they're all 512 ------------------------------ In src/StackExchange.Redis/Enums/ClientType.cs <#1488 (comment)> : > /// /// Replication connections /// - Slave, + [Obsolete(Messages.PreferReplica)] + Slave = 1, + /// + /// Replication connections + /// + Replica = 1, ⬇️ Suggested change - Replica = 1, + Replica = 1, // as an implementation detail, note that enum.ToString prefers *later* options when naming Flags ------------------------------ In src/StackExchange.Redis/Enums/ReplicationChangeOptions.cs <#1488 (comment)> : > EnslaveSubordinates = 4, /// + /// Issue a REPLICAOF to all other known nodes, making this this master of all + /// + ReplicateToSubordinates = 4, // note ToString prefers *later* options FormReplicaChain? I don't know that I have an actual better suggestion here - whatever you think ------------------------------ In src/StackExchange.Redis/Interfaces/IServer.cs <#1488 (comment)> : > bool AllowSlaveWrites { get; set; } + /// + /// Explicitly opt in for replica writes on writable replica + /// + bool AllowReplicaWrites { get; set; } How concerned are we about the interface break here? ------------------------------ In src/StackExchange.Redis/Interfaces/IServer.cs <#1488 (comment)> : > /// + bool IsReplica { get; } Same as below ------------------------------ In src/StackExchange.Redis/RedisServer.cs <#1488 (comment)> : > @@ -638,12 +646,14 @@ public void SlaveOf(EndPoint master, CommandFlags flags = CommandFlags.None) } } - public Task SlaveOfAsync(EndPoint master, CommandFlags flags = CommandFlags.None) + Task IServer.SlaveOfAsync(EndPoint master, CommandFlags flags) => ReplicaOfAsync(master, flags); This went private - public API break when we should obsolete? ------------------------------ In src/StackExchange.Redis/RedisServer.cs <#1488 (comment)> : > @@ -856,14 +866,22 @@ public Task SentinelFailoverAsync(string serviceName, CommandFlags flags = Comma return ExecuteAsync(msg, ResultProcessor.SentinelArrayOfArrays); } - public KeyValuePair<string, string>[][] SentinelSlaves(string serviceName, CommandFlags flags = CommandFlags.None) + KeyValuePair<string, string>[][] IServer.SentinelSlaves(string serviceName, CommandFlags flags) API break? It's a recent addition, but still ------------------------------ In src/StackExchange.Redis/RedisServer.cs <#1488 (comment)> : > var msg = Message.Create(-1, flags, RedisCommand.SENTINEL, RedisLiterals.SLAVES, (RedisValue)serviceName); return ExecuteSync(msg, ResultProcessor.SentinelArrayOfArrays); } - public Task<KeyValuePair<string, string>[][]> SentinelSlavesAsync(string serviceName, CommandFlags flags = CommandFlags.None) + Task<KeyValuePair<string, string>[][]> IServer.SentinelSlavesAsync(string serviceName, CommandFlags flags) API break? It's a recent addition, but still ------------------------------ In src/StackExchange.Redis/ResultProcessor.cs <#1488 (comment)> : > } else if (val.IsEqual(CommonReplies.no)) { - server.SlaveReadOnly = false; - server.Multiplexer.Trace("Auto-configured slave-read-only: false"); + server.ReplicaReadOnly = false; + server.Multiplexer.Trace("Auto-configured replica-read-only: false"); I'm good with this, just noting it's not technically accurate as we could be configuring a pre-5 server and this isn't the config value - don't care, log says what we need? ------------------------------ In src/StackExchange.Redis/ServerEndPoint.cs <#1488 (comment)> : > @@ -642,7 +642,7 @@ internal Task SendTracer(LogProxy log = null) internal string Summary() { var sb = new StringBuilder(Format.ToString(EndPoint)) - .Append(": ").Append(serverType).Append(" v").Append(version).Append(", ").Append(isSlave ? "slave" : "master"); + .Append(": ").Append(serverType).Append(" v").Append(version).Append(", ").Append(isReplica ? "slave" : "master"); ⬇️ Suggested change - .Append(": ").Append(serverType).Append(" v").Append(version).Append(", ").Append(isReplica ? "slave" : "master"); + .Append(": ").Append(serverType).Append(" v").Append(version).Append(", ").Append(isReplica ? "replica" : "master"); ------------------------------ In src/StackExchange.Redis/StringSplits.cs <#1488 (comment)> : > @@ -6,4 +6,9 @@ internal static class StringSplits Space = { ' ' }, Comma = { ',' }; } + + internal static class Messages + { + public const string PreferReplica = "Starting with Redis version 5, redis has moved to 'replica' terminology."; I think this is fine for most places, but probably a better "use this property" instead directly pointing to the replacement would be better - I'm happy to do that commit if cool with it. Related: a "Messages" internal class name here probably isn't awesome, should be ObsoleteMessages or something since Message is a core thing. Cool with me putting specific messages and removing this, bypassing the name issue? — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#1488 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAEHMD5CXPCILPNOXZ3CJ3RVOTCJANCNFSM4NXCWDTQ> .