From 171430a88ff41745edf1f68fc4f51c3108588796 Mon Sep 17 00:00:00 2001 From: slorello89 <42971704+slorello89@users.noreply.github.com> Date: Mon, 18 Apr 2022 15:48:32 -0400 Subject: [PATCH 1/9] updating IsPrimaryOnly command map for new commands --- src/StackExchange.Redis/Message.cs | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/StackExchange.Redis/Message.cs b/src/StackExchange.Redis/Message.cs index 7efd5fe6c..2925ac219 100644 --- a/src/StackExchange.Redis/Message.cs +++ b/src/StackExchange.Redis/Message.cs @@ -338,8 +338,10 @@ public static bool IsPrimaryOnly(RedisCommand command) case RedisCommand.DEL: case RedisCommand.EXPIRE: case RedisCommand.EXPIREAT: + case RedisCommand.EXPIRETIME: case RedisCommand.FLUSHALL: case RedisCommand.FLUSHDB: + case RedisCommand.GEOSEARCHSTORE: case RedisCommand.GETDEL: case RedisCommand.GETEX: case RedisCommand.GETSET: @@ -366,6 +368,7 @@ public static bool IsPrimaryOnly(RedisCommand command) case RedisCommand.PERSIST: case RedisCommand.PEXPIRE: case RedisCommand.PEXPIREAT: + case RedisCommand.PEXPIRETIME: case RedisCommand.PFADD: case RedisCommand.PFMERGE: case RedisCommand.PSETEX: @@ -392,10 +395,12 @@ public static bool IsPrimaryOnly(RedisCommand command) case RedisCommand.TOUCH: case RedisCommand.UNLINK: case RedisCommand.ZADD: + case RedisCommand.ZDIFFSTORE: case RedisCommand.ZINTERSTORE: case RedisCommand.ZINCRBY: case RedisCommand.ZPOPMAX: case RedisCommand.ZPOPMIN: + case RedisCommand.ZRANGESTORE: case RedisCommand.ZREM: case RedisCommand.ZREMRANGEBYLEX: case RedisCommand.ZREMRANGEBYRANK: From c09d2bdf580725e0f737fbb59119f5039a36606a Mon Sep 17 00:00:00 2001 From: slorello89 <42971704+slorello89@users.noreply.github.com> Date: Mon, 18 Apr 2022 15:54:42 -0400 Subject: [PATCH 2/9] more commands --- src/StackExchange.Redis/Message.cs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/StackExchange.Redis/Message.cs b/src/StackExchange.Redis/Message.cs index 2925ac219..ac324edbe 100644 --- a/src/StackExchange.Redis/Message.cs +++ b/src/StackExchange.Redis/Message.cs @@ -333,6 +333,7 @@ public static bool IsPrimaryOnly(RedisCommand command) case RedisCommand.BLPOP: case RedisCommand.BRPOP: case RedisCommand.BRPOPLPUSH: + case RedisCommand.COPY: case RedisCommand.DECR: case RedisCommand.DECRBY: case RedisCommand.DEL: @@ -355,6 +356,7 @@ public static bool IsPrimaryOnly(RedisCommand command) case RedisCommand.INCRBY: case RedisCommand.INCRBYFLOAT: case RedisCommand.LINSERT: + case RedisCommand.LMOVE: case RedisCommand.LPOP: case RedisCommand.LPUSH: case RedisCommand.LPUSHX: From 378679f67a1e8e988e5d200b06d5510e69e5677c Mon Sep 17 00:00:00 2001 From: slorello89 <42971704+slorello89@users.noreply.github.com> Date: Mon, 18 Apr 2022 16:22:03 -0400 Subject: [PATCH 3/9] example replica test --- tests/StackExchange.Redis.Tests/SortedSets.cs | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/tests/StackExchange.Redis.Tests/SortedSets.cs b/tests/StackExchange.Redis.Tests/SortedSets.cs index 0a7f76d9d..b0eca7a87 100644 --- a/tests/StackExchange.Redis.Tests/SortedSets.cs +++ b/tests/StackExchange.Redis.Tests/SortedSets.cs @@ -966,6 +966,22 @@ public void SortedSetRangeStoreFailExclude() Assert.Equal("exclude", exception.ParamName); } + [Fact] + public void SortedSetRangeStoreFailForReplica() + { + using var conn = Create(require: RedisFeatures.v6_2_0); + + var db = conn.GetDatabase(); + var me = Me(); + var sourceKey = $"{me}:ZSetSource"; + var destinationKey = $"{me}:ZSetDestination"; + + db.KeyDelete(new RedisKey[] { sourceKey, destinationKey }, CommandFlags.FireAndForget); + db.SortedSetAdd(sourceKey, lexEntries, CommandFlags.FireAndForget); + var exception = Assert.Throws(() => db.SortedSetRangeAndStore(sourceKey, destinationKey, 0, -1, flags: CommandFlags.DemandReplica)); + Assert.Contains("Command cannot be issued to a replica", exception.Message); + } + [Fact] public void SortedSetScoresSingle() { From f98f899dcbec8a45f21e20a16441e27c16ce149f Mon Sep 17 00:00:00 2001 From: Nick Craver Date: Mon, 18 Apr 2022 17:55:36 -0400 Subject: [PATCH 4/9] Move IsPrimaryOnly to an extension This makes maintenance easier since the place you add a command is right above where it must be a member rather than hidden elsewhere. Tweaking an existing check ensures it works. --- src/StackExchange.Redis/Enums/RedisCommand.cs | 693 ++++++++++++------ src/StackExchange.Redis/Message.cs | 92 +-- tests/StackExchange.Redis.Tests/Naming.cs | 18 +- 3 files changed, 474 insertions(+), 329 deletions(-) diff --git a/src/StackExchange.Redis/Enums/RedisCommand.cs b/src/StackExchange.Redis/Enums/RedisCommand.cs index f7ceb8a1b..ac610cbe1 100644 --- a/src/StackExchange.Redis/Enums/RedisCommand.cs +++ b/src/StackExchange.Redis/Enums/RedisCommand.cs @@ -1,236 +1,465 @@ -namespace StackExchange.Redis +using System; + +namespace StackExchange.Redis; + +internal enum RedisCommand +{ + NONE, // must be first for "zero reasons" + + APPEND, + ASKING, + AUTH, + + BGREWRITEAOF, + BGSAVE, + BITCOUNT, + BITOP, + BITPOS, + BLPOP, + BRPOP, + BRPOPLPUSH, + + CLIENT, + CLUSTER, + CONFIG, + COPY, + + DBSIZE, + DEBUG, + DECR, + DECRBY, + DEL, + DISCARD, + DUMP, + + ECHO, + EVAL, + EVALSHA, + EXEC, + EXISTS, + EXPIRE, + EXPIREAT, + EXPIRETIME, + + FLUSHALL, + FLUSHDB, + + GEOADD, + GEODIST, + GEOHASH, + GEOPOS, + GEORADIUS, + GEORADIUSBYMEMBER, + GEOSEARCH, + GEOSEARCHSTORE, + + GET, + GETBIT, + GETDEL, + GETEX, + GETRANGE, + GETSET, + + HDEL, + HEXISTS, + HGET, + HGETALL, + HINCRBY, + HINCRBYFLOAT, + HKEYS, + HLEN, + HMGET, + HMSET, + HRANDFIELD, + HSCAN, + HSET, + HSETNX, + HSTRLEN, + HVALS, + + INCR, + INCRBY, + INCRBYFLOAT, + INFO, + + KEYS, + + LASTSAVE, + LATENCY, + LINDEX, + LINSERT, + LLEN, + LMOVE, + LPOP, + LPOS, + LPUSH, + LPUSHX, + LRANGE, + LREM, + LSET, + LTRIM, + + MEMORY, + MGET, + MIGRATE, + MONITOR, + MOVE, + MSET, + MSETNX, + MULTI, + + OBJECT, + + PERSIST, + PEXPIRE, + PEXPIREAT, + PEXPIRETIME, + PFADD, + PFCOUNT, + PFMERGE, + PING, + PSETEX, + PSUBSCRIBE, + PTTL, + PUBLISH, + PUBSUB, + PUNSUBSCRIBE, + + QUIT, + + RANDOMKEY, + READONLY, + READWRITE, + RENAME, + RENAMENX, + REPLICAOF, + RESTORE, + ROLE, + RPOP, + RPOPLPUSH, + RPUSH, + RPUSHX, + + SADD, + SAVE, + SCAN, + SCARD, + SCRIPT, + SDIFF, + SDIFFSTORE, + SELECT, + SENTINEL, + SET, + SETBIT, + SETEX, + SETNX, + SETRANGE, + SHUTDOWN, + SINTER, + SINTERCARD, + SINTERSTORE, + SISMEMBER, + SLAVEOF, + SLOWLOG, + SMEMBERS, + SMISMEMBER, + SMOVE, + SORT, + SPOP, + SRANDMEMBER, + SREM, + STRLEN, + SUBSCRIBE, + SUNION, + SUNIONSTORE, + SSCAN, + SWAPDB, + SYNC, + + TIME, + TOUCH, + TTL, + TYPE, + + UNLINK, + UNSUBSCRIBE, + UNWATCH, + + WATCH, + + XACK, + XADD, + XCLAIM, + XDEL, + XGROUP, + XINFO, + XLEN, + XPENDING, + XRANGE, + XREAD, + XREADGROUP, + XREVRANGE, + XTRIM, + + ZADD, + ZCARD, + ZCOUNT, + ZDIFF, + ZDIFFSTORE, + ZINCRBY, + ZINTER, + ZINTERCARD, + ZINTERSTORE, + ZLEXCOUNT, + ZMSCORE, + ZPOPMAX, + ZPOPMIN, + ZRANDMEMBER, + ZRANGE, + ZRANGEBYLEX, + ZRANGEBYSCORE, + ZRANGESTORE, + ZRANK, + ZREM, + ZREMRANGEBYLEX, + ZREMRANGEBYRANK, + ZREMRANGEBYSCORE, + ZREVRANGE, + ZREVRANGEBYLEX, + ZREVRANGEBYSCORE, + ZREVRANK, + ZSCAN, + ZSCORE, + ZUNION, + ZUNIONSTORE, + + UNKNOWN, +} + +internal static class RedisCommandExtensions { - internal enum RedisCommand + /// + /// Gets whether a given command can be issued only to a primary, or if any server is eligible. + /// + /// The to check. + /// if the command is primary-only, otherwise. + [System.Diagnostics.CodeAnalysis.SuppressMessage("Style", "IDE0066:Convert switch statement to expression", Justification = "No, it'd be ridiculous.")] + public static bool IsPrimaryOnly(this RedisCommand command) { - NONE, // must be first for "zero reasons" - - APPEND, - ASKING, - AUTH, - - BGREWRITEAOF, - BGSAVE, - BITCOUNT, - BITOP, - BITPOS, - BLPOP, - BRPOP, - BRPOPLPUSH, - - CLIENT, - CLUSTER, - CONFIG, - COPY, - - DBSIZE, - DEBUG, - DECR, - DECRBY, - DEL, - DISCARD, - DUMP, - - ECHO, - EVAL, - EVALSHA, - EXEC, - EXISTS, - EXPIRE, - EXPIREAT, - EXPIRETIME, - - FLUSHALL, - FLUSHDB, - - GEOADD, - GEODIST, - GEOHASH, - GEOPOS, - GEORADIUS, - GEORADIUSBYMEMBER, - GEOSEARCH, - GEOSEARCHSTORE, - - GET, - GETBIT, - GETDEL, - GETEX, - GETRANGE, - GETSET, - - HDEL, - HEXISTS, - HGET, - HGETALL, - HINCRBY, - HINCRBYFLOAT, - HKEYS, - HLEN, - HMGET, - HMSET, - HRANDFIELD, - HSCAN, - HSET, - HSETNX, - HSTRLEN, - HVALS, - - INCR, - INCRBY, - INCRBYFLOAT, - INFO, - - KEYS, - - LASTSAVE, - LATENCY, - LINDEX, - LINSERT, - LLEN, - LMOVE, - LPOP, - LPOS, - LPUSH, - LPUSHX, - LRANGE, - LREM, - LSET, - LTRIM, - - MEMORY, - MGET, - MIGRATE, - MONITOR, - MOVE, - MSET, - MSETNX, - MULTI, - - OBJECT, - - PERSIST, - PEXPIRE, - PEXPIREAT, - PEXPIRETIME, - PFADD, - PFCOUNT, - PFMERGE, - PING, - PSETEX, - PSUBSCRIBE, - PTTL, - PUBLISH, - PUBSUB, - PUNSUBSCRIBE, - - QUIT, - - RANDOMKEY, - READONLY, - READWRITE, - RENAME, - RENAMENX, - REPLICAOF, - RESTORE, - ROLE, - RPOP, - RPOPLPUSH, - RPUSH, - RPUSHX, - - SADD, - SAVE, - SCAN, - SCARD, - SCRIPT, - SDIFF, - SDIFFSTORE, - SELECT, - SENTINEL, - SET, - SETBIT, - SETEX, - SETNX, - SETRANGE, - SHUTDOWN, - SINTER, - SINTERCARD, - SINTERSTORE, - SISMEMBER, - SLAVEOF, - SLOWLOG, - SMEMBERS, - SMISMEMBER, - SMOVE, - SORT, - SPOP, - SRANDMEMBER, - SREM, - STRLEN, - SUBSCRIBE, - SUNION, - SUNIONSTORE, - SSCAN, - SWAPDB, - SYNC, - - TIME, - TOUCH, - TTL, - TYPE, - - UNLINK, - UNSUBSCRIBE, - UNWATCH, - - WATCH, - - XACK, - XADD, - XCLAIM, - XDEL, - XGROUP, - XINFO, - XLEN, - XPENDING, - XRANGE, - XREAD, - XREADGROUP, - XREVRANGE, - XTRIM, - - ZADD, - ZCARD, - ZCOUNT, - ZDIFF, - ZDIFFSTORE, - ZINCRBY, - ZINTER, - ZINTERCARD, - ZINTERSTORE, - ZLEXCOUNT, - ZMSCORE, - ZPOPMAX, - ZPOPMIN, - ZRANDMEMBER, - ZRANGE, - ZRANGEBYLEX, - ZRANGEBYSCORE, - ZRANGESTORE, - ZRANK, - ZREM, - ZREMRANGEBYLEX, - ZREMRANGEBYRANK, - ZREMRANGEBYSCORE, - ZREVRANGE, - ZREVRANGEBYLEX, - ZREVRANGEBYSCORE, - ZREVRANK, - ZSCAN, - ZSCORE, - ZUNION, - ZUNIONSTORE, - - UNKNOWN, + switch (command) + { + // Commands that can only be issued to a primary (writable) server + case RedisCommand.APPEND: + case RedisCommand.BITOP: + case RedisCommand.BLPOP: + case RedisCommand.BRPOP: + case RedisCommand.BRPOPLPUSH: + case RedisCommand.COPY: + case RedisCommand.DECR: + case RedisCommand.DECRBY: + case RedisCommand.DEL: + case RedisCommand.EXPIRE: + case RedisCommand.EXPIREAT: + case RedisCommand.EXPIRETIME: + case RedisCommand.FLUSHALL: + case RedisCommand.FLUSHDB: + case RedisCommand.GEOADD: + case RedisCommand.GEOSEARCHSTORE: + case RedisCommand.GETDEL: + case RedisCommand.GETEX: + case RedisCommand.GETSET: + case RedisCommand.HDEL: + case RedisCommand.HINCRBY: + case RedisCommand.HINCRBYFLOAT: + case RedisCommand.HMSET: + case RedisCommand.HSET: + case RedisCommand.HSETNX: + case RedisCommand.INCR: + case RedisCommand.INCRBY: + case RedisCommand.INCRBYFLOAT: + case RedisCommand.LINSERT: + case RedisCommand.LMOVE: + case RedisCommand.LPOP: + case RedisCommand.LPUSH: + case RedisCommand.LPUSHX: + case RedisCommand.LREM: + case RedisCommand.LSET: + case RedisCommand.LTRIM: + case RedisCommand.MIGRATE: + case RedisCommand.MOVE: + case RedisCommand.MSET: + case RedisCommand.MSETNX: + case RedisCommand.PERSIST: + case RedisCommand.PEXPIRE: + case RedisCommand.PEXPIREAT: + case RedisCommand.PEXPIRETIME: + case RedisCommand.PFADD: + case RedisCommand.PFMERGE: + case RedisCommand.PSETEX: + case RedisCommand.RENAME: + case RedisCommand.RENAMENX: + case RedisCommand.RESTORE: + case RedisCommand.RPOP: + case RedisCommand.RPOPLPUSH: + case RedisCommand.RPUSH: + case RedisCommand.RPUSHX: + case RedisCommand.SADD: + case RedisCommand.SDIFFSTORE: + case RedisCommand.SET: + case RedisCommand.SETBIT: + case RedisCommand.SETEX: + case RedisCommand.SETNX: + case RedisCommand.SETRANGE: + case RedisCommand.SINTERSTORE: + case RedisCommand.SMOVE: + case RedisCommand.SPOP: + case RedisCommand.SREM: + case RedisCommand.SUNIONSTORE: + case RedisCommand.SWAPDB: + case RedisCommand.TOUCH: + case RedisCommand.UNLINK: + case RedisCommand.ZADD: + case RedisCommand.ZDIFFSTORE: + case RedisCommand.ZINTERSTORE: + case RedisCommand.ZINCRBY: + case RedisCommand.ZPOPMAX: + case RedisCommand.ZPOPMIN: + case RedisCommand.ZRANGESTORE: + case RedisCommand.ZREM: + case RedisCommand.ZREMRANGEBYLEX: + case RedisCommand.ZREMRANGEBYRANK: + case RedisCommand.ZREMRANGEBYSCORE: + case RedisCommand.ZUNIONSTORE: + return true; + // Commands that can be issued anywhere + case RedisCommand.NONE: + case RedisCommand.ASKING: + case RedisCommand.AUTH: + case RedisCommand.BGREWRITEAOF: + case RedisCommand.BGSAVE: + case RedisCommand.BITCOUNT: + case RedisCommand.BITPOS: + case RedisCommand.CLIENT: + case RedisCommand.CLUSTER: + case RedisCommand.CONFIG: + case RedisCommand.DBSIZE: + case RedisCommand.DEBUG: + case RedisCommand.DISCARD: + case RedisCommand.DUMP: + case RedisCommand.ECHO: + case RedisCommand.EVAL: + case RedisCommand.EVALSHA: + case RedisCommand.EXEC: + case RedisCommand.EXISTS: + case RedisCommand.GEODIST: + case RedisCommand.GEOHASH: + case RedisCommand.GEOPOS: + case RedisCommand.GEORADIUS: + case RedisCommand.GEORADIUSBYMEMBER: + case RedisCommand.GEOSEARCH: + case RedisCommand.GET: + case RedisCommand.GETBIT: + case RedisCommand.GETRANGE: + case RedisCommand.HEXISTS: + case RedisCommand.HGET: + case RedisCommand.HGETALL: + case RedisCommand.HKEYS: + case RedisCommand.HLEN: + case RedisCommand.HMGET: + case RedisCommand.HRANDFIELD: + case RedisCommand.HSCAN: + case RedisCommand.HSTRLEN: + case RedisCommand.HVALS: + case RedisCommand.INFO: + case RedisCommand.KEYS: + case RedisCommand.LASTSAVE: + case RedisCommand.LATENCY: + case RedisCommand.LINDEX: + case RedisCommand.LLEN: + case RedisCommand.LPOS: + case RedisCommand.LRANGE: + case RedisCommand.MEMORY: + case RedisCommand.MGET: + case RedisCommand.MONITOR: + case RedisCommand.MULTI: + case RedisCommand.OBJECT: + case RedisCommand.PFCOUNT: + case RedisCommand.PING: + case RedisCommand.PSUBSCRIBE: + case RedisCommand.PTTL: + case RedisCommand.PUBLISH: + case RedisCommand.PUBSUB: + case RedisCommand.PUNSUBSCRIBE: + case RedisCommand.QUIT: + case RedisCommand.RANDOMKEY: + case RedisCommand.READONLY: + case RedisCommand.READWRITE: + case RedisCommand.REPLICAOF: + case RedisCommand.ROLE: + case RedisCommand.SAVE: + case RedisCommand.SCAN: + case RedisCommand.SCARD: + case RedisCommand.SCRIPT: + case RedisCommand.SDIFF: + case RedisCommand.SELECT: + case RedisCommand.SENTINEL: + case RedisCommand.SHUTDOWN: + case RedisCommand.SINTER: + case RedisCommand.SINTERCARD: + case RedisCommand.SISMEMBER: + case RedisCommand.SLAVEOF: + case RedisCommand.SLOWLOG: + case RedisCommand.SMEMBERS: + case RedisCommand.SMISMEMBER: + case RedisCommand.SORT: + case RedisCommand.SRANDMEMBER: + case RedisCommand.STRLEN: + case RedisCommand.SUBSCRIBE: + case RedisCommand.SUNION: + case RedisCommand.SSCAN: + case RedisCommand.SYNC: + case RedisCommand.TIME: + case RedisCommand.TTL: + case RedisCommand.TYPE: + case RedisCommand.UNSUBSCRIBE: + case RedisCommand.UNWATCH: + case RedisCommand.WATCH: + case RedisCommand.XACK: + case RedisCommand.XADD: + case RedisCommand.XCLAIM: + case RedisCommand.XDEL: + case RedisCommand.XGROUP: + case RedisCommand.XINFO: + case RedisCommand.XLEN: + case RedisCommand.XPENDING: + case RedisCommand.XRANGE: + case RedisCommand.XREAD: + case RedisCommand.XREADGROUP: + case RedisCommand.XREVRANGE: + case RedisCommand.XTRIM: + case RedisCommand.ZCARD: + case RedisCommand.ZCOUNT: + case RedisCommand.ZDIFF: + case RedisCommand.ZINTER: + case RedisCommand.ZINTERCARD: + case RedisCommand.ZLEXCOUNT: + case RedisCommand.ZMSCORE: + case RedisCommand.ZRANDMEMBER: + case RedisCommand.ZRANGE: + case RedisCommand.ZRANGEBYLEX: + case RedisCommand.ZRANGEBYSCORE: + case RedisCommand.ZRANK: + case RedisCommand.ZREVRANGE: + case RedisCommand.ZREVRANGEBYLEX: + case RedisCommand.ZREVRANGEBYSCORE: + case RedisCommand.ZREVRANK: + case RedisCommand.ZSCAN: + case RedisCommand.ZSCORE: + case RedisCommand.ZUNION: + case RedisCommand.UNKNOWN: + return false; + default: + throw new ArgumentOutOfRangeException(nameof(command), $"Every RedisCommand must be defined in Message.IsPrimaryOnly, unknown command '{command}' encountered."); + } } } diff --git a/src/StackExchange.Redis/Message.cs b/src/StackExchange.Redis/Message.cs index ac324edbe..f44a395d5 100644 --- a/src/StackExchange.Redis/Message.cs +++ b/src/StackExchange.Redis/Message.cs @@ -106,7 +106,7 @@ protected Message(int db, CommandFlags flags, RedisCommand command) } } - bool primaryOnly = IsPrimaryOnly(command); + bool primaryOnly = command.IsPrimaryOnly(); Db = db; this.command = command; Flags = flags & UserSelectableFlags; @@ -324,96 +324,6 @@ public static Message Create(int db, CommandFlags flags, RedisCommand command, i public static Message CreateInSlot(int db, int slot, CommandFlags flags, RedisCommand command, RedisValue[] values) => new CommandSlotValuesMessage(db, slot, flags, command, values); - public static bool IsPrimaryOnly(RedisCommand command) - { - switch (command) - { - case RedisCommand.APPEND: - case RedisCommand.BITOP: - case RedisCommand.BLPOP: - case RedisCommand.BRPOP: - case RedisCommand.BRPOPLPUSH: - case RedisCommand.COPY: - case RedisCommand.DECR: - case RedisCommand.DECRBY: - case RedisCommand.DEL: - case RedisCommand.EXPIRE: - case RedisCommand.EXPIREAT: - case RedisCommand.EXPIRETIME: - case RedisCommand.FLUSHALL: - case RedisCommand.FLUSHDB: - case RedisCommand.GEOSEARCHSTORE: - case RedisCommand.GETDEL: - case RedisCommand.GETEX: - case RedisCommand.GETSET: - case RedisCommand.HDEL: - case RedisCommand.HINCRBY: - case RedisCommand.HINCRBYFLOAT: - case RedisCommand.HMSET: - case RedisCommand.HSET: - case RedisCommand.HSETNX: - case RedisCommand.INCR: - case RedisCommand.INCRBY: - case RedisCommand.INCRBYFLOAT: - case RedisCommand.LINSERT: - case RedisCommand.LMOVE: - case RedisCommand.LPOP: - case RedisCommand.LPUSH: - case RedisCommand.LPUSHX: - case RedisCommand.LREM: - case RedisCommand.LSET: - case RedisCommand.LTRIM: - case RedisCommand.MIGRATE: - case RedisCommand.MOVE: - case RedisCommand.MSET: - case RedisCommand.MSETNX: - case RedisCommand.PERSIST: - case RedisCommand.PEXPIRE: - case RedisCommand.PEXPIREAT: - case RedisCommand.PEXPIRETIME: - case RedisCommand.PFADD: - case RedisCommand.PFMERGE: - case RedisCommand.PSETEX: - case RedisCommand.RENAME: - case RedisCommand.RENAMENX: - case RedisCommand.RESTORE: - case RedisCommand.RPOP: - case RedisCommand.RPOPLPUSH: - case RedisCommand.RPUSH: - case RedisCommand.RPUSHX: - case RedisCommand.SADD: - case RedisCommand.SDIFFSTORE: - case RedisCommand.SET: - case RedisCommand.SETBIT: - case RedisCommand.SETEX: - case RedisCommand.SETNX: - case RedisCommand.SETRANGE: - case RedisCommand.SINTERSTORE: - case RedisCommand.SMOVE: - case RedisCommand.SPOP: - case RedisCommand.SREM: - case RedisCommand.SUNIONSTORE: - case RedisCommand.SWAPDB: - case RedisCommand.TOUCH: - case RedisCommand.UNLINK: - case RedisCommand.ZADD: - case RedisCommand.ZDIFFSTORE: - case RedisCommand.ZINTERSTORE: - case RedisCommand.ZINCRBY: - case RedisCommand.ZPOPMAX: - case RedisCommand.ZPOPMIN: - case RedisCommand.ZRANGESTORE: - case RedisCommand.ZREM: - case RedisCommand.ZREMRANGEBYLEX: - case RedisCommand.ZREMRANGEBYRANK: - case RedisCommand.ZREMRANGEBYSCORE: - case RedisCommand.ZUNIONSTORE: - return true; - default: - return false; - } - } - /// Gets whether this is primary-only. /// /// Note that the constructor runs the switch statement above, so diff --git a/tests/StackExchange.Redis.Tests/Naming.cs b/tests/StackExchange.Redis.Tests/Naming.cs index 224d09b5f..55997848c 100644 --- a/tests/StackExchange.Redis.Tests/Naming.cs +++ b/tests/StackExchange.Redis.Tests/Naming.cs @@ -28,14 +28,17 @@ public void CheckSignatures(Type type, bool isAsync) } } + /// + /// This test iterates over all s to ensure we have everything accounted for as primary-only or not. + /// [Fact] - public void ShowReadOnlyOperations() + public void CheckReadOnlyOperations() { - List primaryReplica = new List(); - List primaryOnly = new List(); + List primaryReplica = new(), + primaryOnly = new(); foreach (var val in (RedisCommand[])Enum.GetValues(typeof(RedisCommand))) { - bool isPrimaryOnly = Message.IsPrimaryOnly(val); + bool isPrimaryOnly = val.IsPrimaryOnly(); (isPrimaryOnly ? primaryOnly : primaryReplica).Add(val); if (!isPrimaryOnly) @@ -43,18 +46,21 @@ public void ShowReadOnlyOperations() Log(val.ToString()); } } + // Ensure an unknown command from nowhere would violate the check above, as any not-yet-added one would. + Assert.Throws(() => ((RedisCommand)99999).IsPrimaryOnly()); + Log("primary-only: {0}, vs primary/replica: {1}", primaryOnly.Count, primaryReplica.Count); Log(""); Log("primary-only:"); foreach (var val in primaryOnly) { - Log(val?.ToString()); + Log(val.ToString()); } Log(""); Log("primary/replica:"); foreach (var val in primaryReplica) { - Log(val?.ToString()); + Log(val.ToString()); } } From f1ce3128f2b19191b26fad4d602fda2df92d4b96 Mon Sep 17 00:00:00 2001 From: Nick Craver Date: Mon, 18 Apr 2022 20:24:55 -0400 Subject: [PATCH 5/9] Beef up comments Goal is making contributing easier, so more comments here! --- src/StackExchange.Redis/Enums/RedisCommand.cs | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/StackExchange.Redis/Enums/RedisCommand.cs b/src/StackExchange.Redis/Enums/RedisCommand.cs index ac610cbe1..42740467c 100644 --- a/src/StackExchange.Redis/Enums/RedisCommand.cs +++ b/src/StackExchange.Redis/Enums/RedisCommand.cs @@ -249,6 +249,10 @@ public static bool IsPrimaryOnly(this RedisCommand command) switch (command) { // Commands that can only be issued to a primary (writable) server + // If a command *may* be writable (e.g. an EVAL script), it should *not* be primary-only + // because that'd block a legitimate use case of a read-only script on replica servers, + // for example spreading load via a .DemandReplica flag in the caller. + // Basically: would it fail on a read-only replica in 100% of cases? Then it goes in the list. case RedisCommand.APPEND: case RedisCommand.BITOP: case RedisCommand.BLPOP: From f5476a963ae878e7331876d7b96034ae21ea441c Mon Sep 17 00:00:00 2001 From: Nick Craver Date: Mon, 18 Apr 2022 21:10:53 -0400 Subject: [PATCH 6/9] Improve exceptions for read-only things! --- src/StackExchange.Redis/Enums/RedisCommand.cs | 2 +- src/StackExchange.Redis/ExceptionFactory.cs | 5 +++++ tests/StackExchange.Redis.Tests/AsyncTests.cs | 2 +- .../ExceptionFactoryTests.cs | 15 +++++++++++++++ tests/StackExchange.Redis.Tests/Streams.cs | 10 ++++++++++ 5 files changed, 32 insertions(+), 2 deletions(-) diff --git a/src/StackExchange.Redis/Enums/RedisCommand.cs b/src/StackExchange.Redis/Enums/RedisCommand.cs index 42740467c..26a4e11f7 100644 --- a/src/StackExchange.Redis/Enums/RedisCommand.cs +++ b/src/StackExchange.Redis/Enums/RedisCommand.cs @@ -322,6 +322,7 @@ public static bool IsPrimaryOnly(this RedisCommand command) case RedisCommand.SWAPDB: case RedisCommand.TOUCH: case RedisCommand.UNLINK: + case RedisCommand.XADD: case RedisCommand.ZADD: case RedisCommand.ZDIFFSTORE: case RedisCommand.ZINTERSTORE: @@ -429,7 +430,6 @@ public static bool IsPrimaryOnly(this RedisCommand command) case RedisCommand.UNWATCH: case RedisCommand.WATCH: case RedisCommand.XACK: - case RedisCommand.XADD: case RedisCommand.XCLAIM: case RedisCommand.XDEL: case RedisCommand.XGROUP: diff --git a/src/StackExchange.Redis/ExceptionFactory.cs b/src/StackExchange.Redis/ExceptionFactory.cs index 7200a3a06..5cb219f97 100644 --- a/src/StackExchange.Redis/ExceptionFactory.cs +++ b/src/StackExchange.Redis/ExceptionFactory.cs @@ -124,6 +124,11 @@ internal static Exception NoConnectionAvailable( // This can happen in cloud environments often, where user disables abort and has the wrong config initialMessage = $"Connection to Redis never succeeded (attempts: {attempts} - check your config), unable to service operation: "; } + else if (message is not null && message.IsPrimaryOnly()) + { + // If we know it's a primary-only command, indicate that in the error message + initialMessage = "No connection (requires writable - not eligible for replica) is active/available to service this operation: "; + } else { // Default if we don't have a more useful error message here based on circumstances diff --git a/tests/StackExchange.Redis.Tests/AsyncTests.cs b/tests/StackExchange.Redis.Tests/AsyncTests.cs index e754cda7c..0d6e3bb98 100644 --- a/tests/StackExchange.Redis.Tests/AsyncTests.cs +++ b/tests/StackExchange.Redis.Tests/AsyncTests.cs @@ -39,7 +39,7 @@ public void AsyncTasksReportFailureIfServerUnavailable() Assert.NotNull(c.Exception); var ex = c.Exception.InnerExceptions.Single(); Assert.IsType(ex); - Assert.StartsWith("No connection is active/available to service this operation: SADD " + key.ToString(), ex.Message); + Assert.StartsWith("No connection (requires writable - not eligible for replica) is active/available to service this operation: SADD " + key.ToString(), ex.Message); } [Fact] diff --git a/tests/StackExchange.Redis.Tests/ExceptionFactoryTests.cs b/tests/StackExchange.Redis.Tests/ExceptionFactoryTests.cs index 1098a034c..cb34aa19c 100644 --- a/tests/StackExchange.Redis.Tests/ExceptionFactoryTests.cs +++ b/tests/StackExchange.Redis.Tests/ExceptionFactoryTests.cs @@ -208,6 +208,21 @@ public void NoConnectionException(bool abortOnConnect, int connCount, int comple } } + [Fact] + public void NoConnectionPrimaryOnlyException() + { + using var conn = ConnectionMultiplexer.Connect(TestConfig.Current.ReplicaServerAndPort, Writer); + + var msg = Message.Create(0, CommandFlags.None, RedisCommand.SET, (RedisKey)Me(), (RedisValue)"test"); + Assert.True(msg.IsPrimaryOnly()); + var rawEx = ExceptionFactory.NoConnectionAvailable(conn, msg, null); + var ex = Assert.IsType(rawEx); + Writer.WriteLine("Exception: " + ex.Message); + + // Ensure a primary-only operation like SET gives the additional context + Assert.StartsWith("No connection (requires writable - not eligible for replica) is active/available to service this operation: SET", ex.Message); + } + [Theory] [InlineData(true, ConnectionFailureType.ProtocolFailure, "ProtocolFailure on [0]:GET myKey (StringProcessor), my annotation")] [InlineData(true, ConnectionFailureType.ConnectionDisposed, "ConnectionDisposed on [0]:GET myKey (StringProcessor), my annotation")] diff --git a/tests/StackExchange.Redis.Tests/Streams.cs b/tests/StackExchange.Redis.Tests/Streams.cs index adb5511dd..413fa7935 100644 --- a/tests/StackExchange.Redis.Tests/Streams.cs +++ b/tests/StackExchange.Redis.Tests/Streams.cs @@ -26,6 +26,16 @@ public void IsStreamType() Assert.Equal(RedisType.Stream, keyType); } + [Fact] + public void StreamOpsFailOnReplica() + { + using var conn = Create(configuration: TestConfig.Current.ReplicaServerAndPort, require: RedisFeatures.v5_0_0); + + var db = conn.GetDatabase(); + var ex = Assert.Throws(() => db.StreamAdd(GetUniqueKey("auto_id"), "field1", "value1")); + Assert.StartsWith("No connection (requires writable - not eligible for replica) is active/available", ex.Message); + } + [Fact] public void StreamAddSinglePairWithAutoId() { From efefeb0bca1d031cf40d95980d1ea26c51e24f98 Mon Sep 17 00:00:00 2001 From: Nick Craver Date: Mon, 18 Apr 2022 21:49:20 -0400 Subject: [PATCH 7/9] RedisCommand: fix streams and add test for 'em --- src/StackExchange.Redis/Enums/RedisCommand.cs | 13 +-- tests/StackExchange.Redis.Tests/Streams.cs | 82 ++++++++++++++++++- 2 files changed, 87 insertions(+), 8 deletions(-) diff --git a/src/StackExchange.Redis/Enums/RedisCommand.cs b/src/StackExchange.Redis/Enums/RedisCommand.cs index 26a4e11f7..5a49ef9e6 100644 --- a/src/StackExchange.Redis/Enums/RedisCommand.cs +++ b/src/StackExchange.Redis/Enums/RedisCommand.cs @@ -322,7 +322,13 @@ public static bool IsPrimaryOnly(this RedisCommand command) case RedisCommand.SWAPDB: case RedisCommand.TOUCH: case RedisCommand.UNLINK: + case RedisCommand.XACK: case RedisCommand.XADD: + case RedisCommand.XCLAIM: + case RedisCommand.XDEL: + case RedisCommand.XGROUP: + case RedisCommand.XREADGROUP: + case RedisCommand.XTRIM: case RedisCommand.ZADD: case RedisCommand.ZDIFFSTORE: case RedisCommand.ZINTERSTORE: @@ -429,18 +435,13 @@ public static bool IsPrimaryOnly(this RedisCommand command) case RedisCommand.UNSUBSCRIBE: case RedisCommand.UNWATCH: case RedisCommand.WATCH: - case RedisCommand.XACK: - case RedisCommand.XCLAIM: - case RedisCommand.XDEL: - case RedisCommand.XGROUP: + // Stream commands verified working on replicas case RedisCommand.XINFO: case RedisCommand.XLEN: case RedisCommand.XPENDING: case RedisCommand.XRANGE: case RedisCommand.XREAD: - case RedisCommand.XREADGROUP: case RedisCommand.XREVRANGE: - case RedisCommand.XTRIM: case RedisCommand.ZCARD: case RedisCommand.ZCOUNT: case RedisCommand.ZDIFF: diff --git a/tests/StackExchange.Redis.Tests/Streams.cs b/tests/StackExchange.Redis.Tests/Streams.cs index 413fa7935..c946b1cf7 100644 --- a/tests/StackExchange.Redis.Tests/Streams.cs +++ b/tests/StackExchange.Redis.Tests/Streams.cs @@ -29,10 +29,88 @@ public void IsStreamType() [Fact] public void StreamOpsFailOnReplica() { - using var conn = Create(configuration: TestConfig.Current.ReplicaServerAndPort, require: RedisFeatures.v5_0_0); + using var conn = Create(configuration: TestConfig.Current.PrimaryServerAndPort, require: RedisFeatures.v5_0_0); + using var replicaConn = Create(configuration: TestConfig.Current.ReplicaServerAndPort, require: RedisFeatures.v5_0_0); var db = conn.GetDatabase(); - var ex = Assert.Throws(() => db.StreamAdd(GetUniqueKey("auto_id"), "field1", "value1")); + var replicaDb = replicaConn.GetDatabase(); + + // XADD: Works on primary, not secondary + db.StreamAdd(GetUniqueKey("auto_id"), "field1", "value1"); + var ex = Assert.Throws(() => replicaDb.StreamAdd(GetUniqueKey("auto_id"), "field1", "value1")); + Assert.StartsWith("No connection (requires writable - not eligible for replica) is active/available", ex.Message); + + // Add stream content to primary + var key = GetUniqueKey("group_ack"); + const string groupName1 = "test_group1", + groupName2 = "test_group2", + consumer1 = "test_consumer1", + consumer2 = "test_consumer2"; + + // Add for primary + var id1 = db.StreamAdd(key, "field1", "value1"); + var id2 = db.StreamAdd(key, "field2", "value2"); + var id3 = db.StreamAdd(key, "field3", "value3"); + var id4 = db.StreamAdd(key, "field4", "value4"); + + // XGROUP: Works on primary, not replica + db.StreamCreateConsumerGroup(key, groupName1, StreamPosition.Beginning); + ex = Assert.Throws(() => replicaDb.StreamCreateConsumerGroup(key, groupName2, StreamPosition.Beginning)); + Assert.StartsWith("No connection (requires writable - not eligible for replica) is active/available", ex.Message); + + // Create the second group on the primary, for the rest of the tests. + db.StreamCreateConsumerGroup(key, groupName2, StreamPosition.Beginning); + + // XREADGROUP: Works on primary, not replica + // Read all 4 messages, they will be assigned to the consumer + var entries = db.StreamReadGroup(key, groupName1, consumer1, StreamPosition.NewMessages); + ex = Assert.Throws(() => replicaDb.StreamReadGroup(key, groupName2, consumer2, StreamPosition.NewMessages)); + Assert.StartsWith("No connection (requires writable - not eligible for replica) is active/available", ex.Message); + + // XACK: Works on primary, not secondary + var oneAck = db.StreamAcknowledge(key, groupName1, id1); + ex = Assert.Throws(() => replicaDb.StreamAcknowledge(key, groupName2, id1)); + Assert.StartsWith("No connection (requires writable - not eligible for replica) is active/available", ex.Message); + + // XPENDING: Works on primary and replica + // Get the pending messages for consumer2. + var pendingMessages = db.StreamPendingMessages(key, groupName1, 10, consumer1); + var pendingMessages2 = replicaDb.StreamPendingMessages(key, groupName2, 10, consumer2); + + // XCLAIM: Works on primary, not replica + // Claim the messages for consumer1. + var messages = db.StreamClaim(key, groupName1, consumer1, 0, messageIds: pendingMessages.Select(pm => pm.MessageId).ToArray()); + ex = Assert.Throws(() => replicaDb.StreamClaim(key, groupName2, consumer2, 0, messageIds: pendingMessages.Select(pm => pm.MessageId).ToArray())); + Assert.StartsWith("No connection (requires writable - not eligible for replica) is active/available", ex.Message); + + // XDEL: Works on primary, not replica + db.StreamDelete(key, new RedisValue[] { id4 }); + ex = Assert.Throws(() => replicaDb.StreamDelete(key, new RedisValue[] { id3 })); + Assert.StartsWith("No connection (requires writable - not eligible for replica) is active/available", ex.Message); + + // XINFO: Works on primary and replica + db.StreamInfo(key); + replicaDb.StreamInfo(key); + + // XLEN: Works on primary and replica + db.StreamLength(key); + replicaDb.StreamLength(key); + + // XRANGE: Works on primary and replica + db.StreamRange(key); + replicaDb.StreamRange(key); + + // XREVRANGE: Works on primary and replica + db.StreamRange(key, messageOrder: Order.Descending); + replicaDb.StreamRange(key, messageOrder: Order.Descending); + + // XREAD: Works on primary and replica + db.StreamRead(key, "0-1"); + replicaDb.StreamRead(key, "0-1"); + + // XTRIM: Works on primary, not replica + db.StreamTrim(key, 10); + ex = Assert.Throws(() => replicaDb.StreamTrim(key, 10)); Assert.StartsWith("No connection (requires writable - not eligible for replica) is active/available", ex.Message); } From 54e72b4b017232e5270693c8b385af5858de8adc Mon Sep 17 00:00:00 2001 From: Nick Craver Date: Mon, 18 Apr 2022 22:11:34 -0400 Subject: [PATCH 8/9] Internal Can apply the same love to many other classes to be clear but will make another PR for it. --- src/StackExchange.Redis/Enums/RedisCommand.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/StackExchange.Redis/Enums/RedisCommand.cs b/src/StackExchange.Redis/Enums/RedisCommand.cs index 5a49ef9e6..e2cc9603c 100644 --- a/src/StackExchange.Redis/Enums/RedisCommand.cs +++ b/src/StackExchange.Redis/Enums/RedisCommand.cs @@ -244,7 +244,7 @@ internal static class RedisCommandExtensions /// The to check. /// if the command is primary-only, otherwise. [System.Diagnostics.CodeAnalysis.SuppressMessage("Style", "IDE0066:Convert switch statement to expression", Justification = "No, it'd be ridiculous.")] - public static bool IsPrimaryOnly(this RedisCommand command) + internal static bool IsPrimaryOnly(this RedisCommand command) { switch (command) { From b2c538d7c60d23e7feeafc2ff22dde321a0cabd9 Mon Sep 17 00:00:00 2001 From: Nick Craver Date: Mon, 18 Apr 2022 22:27:10 -0400 Subject: [PATCH 9/9] Restrict message more --- src/StackExchange.Redis/ExceptionFactory.cs | 2 +- tests/StackExchange.Redis.Tests/AsyncTests.cs | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/StackExchange.Redis/ExceptionFactory.cs b/src/StackExchange.Redis/ExceptionFactory.cs index 5cb219f97..2c0f2b5ca 100644 --- a/src/StackExchange.Redis/ExceptionFactory.cs +++ b/src/StackExchange.Redis/ExceptionFactory.cs @@ -124,7 +124,7 @@ internal static Exception NoConnectionAvailable( // This can happen in cloud environments often, where user disables abort and has the wrong config initialMessage = $"Connection to Redis never succeeded (attempts: {attempts} - check your config), unable to service operation: "; } - else if (message is not null && message.IsPrimaryOnly()) + else if (message is not null && message.IsPrimaryOnly() && multiplexer.IsConnected) { // If we know it's a primary-only command, indicate that in the error message initialMessage = "No connection (requires writable - not eligible for replica) is active/available to service this operation: "; diff --git a/tests/StackExchange.Redis.Tests/AsyncTests.cs b/tests/StackExchange.Redis.Tests/AsyncTests.cs index 0d6e3bb98..e754cda7c 100644 --- a/tests/StackExchange.Redis.Tests/AsyncTests.cs +++ b/tests/StackExchange.Redis.Tests/AsyncTests.cs @@ -39,7 +39,7 @@ public void AsyncTasksReportFailureIfServerUnavailable() Assert.NotNull(c.Exception); var ex = c.Exception.InnerExceptions.Single(); Assert.IsType(ex); - Assert.StartsWith("No connection (requires writable - not eligible for replica) is active/available to service this operation: SADD " + key.ToString(), ex.Message); + Assert.StartsWith("No connection is active/available to service this operation: SADD " + key.ToString(), ex.Message); } [Fact]